feat: Add expense service with endpoints for creating, listing, and managing expenses #23
Conversation
…anaging expenses - Integrated expense routes into the main application. - Created test suite for expense service, including API endpoint tests and validation checks. - Implemented PATCH endpoint validation for updating expenses. - Developed algorithms for normal and advanced settlement calculations. - Added unit tests for expense split validation and settlement algorithms. - Established directory structure for organizing expense-related tests.
✅ Deploy Preview for splitwizer canceled.
|
WalkthroughA comprehensive expense management service was introduced, including core CRUD, settlement, analytics, and user balance endpoints, along with detailed Pydantic schemas, service logic, and route handlers. Validation, error handling, and testing were enhanced. Group member APIs now return enriched user details. Extensive documentation and completion summaries were added. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant API_Router
participant ExpenseService
participant DB
participant UserService
Client->>API_Router: PATCH /groups/{group_id}/expenses/{expense_id}
API_Router->>ExpenseService: update_expense(group_id, expense_id, updates, user_id)
ExpenseService->>DB: Fetch expense by expense_id
ExpenseService->>DB: Validate and update expense (separate $set/$push)
ExpenseService->>DB: Recalculate settlements if needed
ExpenseService->>DB: Save updated expense and settlements
ExpenseService->>API_Router: Return updated ExpenseResponse
API_Router->>Client: Respond with updated expense
Note over API_Router,ExpenseService: Error handling and validation at each step
sequenceDiagram
participant Client
participant API_Router
participant ExpenseService
participant DB
participant UserService
Client->>API_Router: GET /groups/{group_id}/members
API_Router->>ExpenseService: get_group_members(group_id, user_id)
ExpenseService->>DB: Fetch group document
ExpenseService->>UserService: For each member, fetch user profile
UserService-->>ExpenseService: Return user details
ExpenseService->>API_Router: Return enriched member list
API_Router->>Client: Respond with detailed member data
Suggested labels
Poem
✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
…, and debug functionality
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🔭 Outside diff range comments (1)
backend/app/expenses/routes.py (1)
373-419: Security consideration for debug endpoint.The debug endpoint exposes internal details and should be secured or removed before production deployment. Consider:
- Adding environment-based conditional inclusion
- Restricting to admin users only
- Adding rate limiting
# Only include debug endpoint in development if settings.ENVIRONMENT == "development": @router.get("/expenses/{expense_id}/debug") async def debug_expense(...): # existing implementation
🧹 Nitpick comments (14)
backend/tests/expenses/test_expense_service.py (1)
144-146: Fix formatting issues.Add proper spacing after the function definition according to PEP 8.
Apply this diff:
if __name__ == "__main__": pytest.main([__file__]) +backend/test_patch_endpoint.py (2)
6-8: Remove unused import.The
SplitTypeimport is unused according to static analysis.Apply this diff:
-from app.expenses.schemas import ExpenseUpdateRequest, ExpenseSplit, SplitType +from app.expenses.schemas import ExpenseUpdateRequest, ExpenseSplit
103-120: Fix formatting and improve debugging information.Add proper spacing and fix the f-string without placeholders.
Apply this diff:
+ if __name__ == "__main__": asyncio.run(test_patch_validation()) test_mongodb_update_structure() print("\n💡 Common PATCH endpoint issues:") print(" 1. Validator errors with partial updates") print(" 2. MongoDB $set and $push conflicts") print(" 3. Missing fields in request validation") print(" 4. ObjectId conversion issues") print(" 5. Authorization/authentication problems") print("\n🔧 To debug the 500 error:") print(" 1. Check server logs for detailed error messages") print(" 2. Test with a simple update (description only)") print(" 3. Verify the expense ID and group ID are valid") print(" 4. Ensure user has permission to edit the expense") - print(" 5. Check MongoDB connection and collection names") + print(" 5. Check MongoDB connection and collection names")backend/test_expense_service.py (2)
7-11: Remove unused imports.Both
jsonanddatetimeimports are unused according to static analysis.Apply this diff:
import requests -import json -from datetime import datetime BASE_URL = "http://localhost:8000"
250-252: Fix formatting after function definition.Add proper spacing according to PEP 8.
Apply this diff:
if __name__ == "__main__": test_expense_apis() +backend/tests/expenses/test_expense_routes.py (1)
146-148: Fix formatting after function definition.Add proper spacing according to PEP 8.
Apply this diff:
if __name__ == "__main__": pytest.main([__file__]) +backend/app/expenses/README.md (3)
53-60: Add language specification to code blocks for better syntax highlighting.The API endpoint code blocks are missing language specifications, which affects readability and syntax highlighting.
Apply this diff to add language specifications:
-``` +```http POST /groups/{group_id}/expenses # Create expense GET /groups/{group_id}/expenses # List expenses GET /groups/{group_id}/expenses/{expense_id} # Get single expense PATCH /groups/{group_id}/expenses/{expense_id} # Update expense DELETE /groups/{group_id}/expenses/{expense_id} # Delete expenseAnd similarly for the other endpoint blocks at lines 62-66, 68-76, and 78-84. Also applies to: 62-66, 68-76, 78-84 --- `142-142`: **Fix grammar: add missing article.** ```diff -- Split amounts must sum to total expense amount (±0.01 tolerance) +- Split amounts must sum to the total expense amount (±0.01 tolerance)
209-209: Add newline at end of file.Files should end with a newline character for POSIX compliance.
- Edge cases and error conditions +- Edge cases and error conditions +backend/PATCH_FIX_SUMMARY.md (2)
6-6: Fix grammar: add missing article.-**Problem**: Using `$push` inside `$set` operation caused MongoDB error. +**Problem**: Using `$push` inside `$set` operation caused a MongoDB error.
118-118: Add newline at end of file.Files should end with a newline character for POSIX compliance.
## The PATCH endpoint should now work correctly without 500 errors! +backend/app/expenses/service.py (3)
12-15: Add blank line before class and improve empty init.from collections import defaultdict + class ExpenseService: def __init__(self): - pass + """Initialize ExpenseService.""" + pass
978-1084: Consider refactoring complex analytics method.The
get_group_analyticsmethod is quite complex with 31 local variables and 16 branches. Consider breaking it down into smaller, focused methods:
_build_date_range(period, year, month)_analyze_categories(expenses, total_expenses)_calculate_member_contributions(expenses, group_members)_generate_expense_trends(expenses, start_date, end_date)This would improve maintainability and testability.
Would you like me to generate a refactored version with these smaller methods?
1085-1087: Add blank line before module-level code."expenseTrends": expense_trends } + # Create service instance expense_service = ExpenseService()
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
backend/PATCH_FIX_SUMMARY.md(1 hunks)backend/app/expenses/README.md(1 hunks)backend/app/expenses/routes.py(1 hunks)backend/app/expenses/schemas.py(1 hunks)backend/app/expenses/service.py(1 hunks)backend/main.py(2 hunks)backend/test_expense_service.py(1 hunks)backend/test_patch_endpoint.py(1 hunks)backend/tests/expenses/test_expense_routes.py(1 hunks)backend/tests/expenses/test_expense_service.py(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (3)
backend/test_patch_endpoint.py (1)
backend/app/expenses/schemas.py (3)
ExpenseUpdateRequest(37-55)ExpenseSplit(16-19)SplitType(6-9)
backend/tests/expenses/test_expense_routes.py (2)
backend/app/expenses/schemas.py (2)
ExpenseCreateRequest(21-35)ExpenseSplit(16-19)backend/tests/user/test_user_routes.py (1)
client(13-15)
backend/app/expenses/service.py (2)
backend/app/expenses/schemas.py (8)
ExpenseCreateRequest(21-35)ExpenseUpdateRequest(37-55)ExpenseResponse(75-90)Settlement(92-106)OptimizedSettlement(108-114)SettlementCreateRequest(131-136)SettlementStatus(11-14)SplitType(6-9)backend/app/expenses/routes.py (7)
create_expense(21-33)calculate_optimized_settlements(282-310)list_group_expenses(36-55)update_expense(73-89)delete_expense(92-106)get_group_settlements(180-225)delete_settlement(261-279)
🪛 Ruff (0.11.9)
backend/tests/expenses/test_expense_service.py
4-4: app.expenses.service.expense_service imported but unused
Remove unused import: app.expenses.service.expense_service
(F401)
6-6: asyncio imported but unused
Remove unused import: asyncio
(F401)
14-14: Local variable group_id is assigned to but never used
Remove assignment to unused variable group_id
(F841)
17-17: Local variable settlements is assigned to but never used
Remove assignment to unused variable settlements
(F841)
38-38: Local variable user_balances is assigned to but never used
Remove assignment to unused variable user_balances
(F841)
backend/test_expense_service.py
8-8: json imported but unused
Remove unused import: json
(F401)
9-9: datetime.datetime imported but unused
Remove unused import: datetime.datetime
(F401)
backend/test_patch_endpoint.py
7-7: app.expenses.schemas.SplitType imported but unused
Remove unused import: app.expenses.schemas.SplitType
(F401)
54-54: Local variable e is assigned to but never used
Remove assignment to unused variable e
(F841)
65-65: Local variable update_request is assigned to but never used
Remove assignment to unused variable update_request
(F841)
93-93: Local variable mongodb_update is assigned to but never used
Remove assignment to unused variable mongodb_update
(F841)
100-100: f-string without any placeholders
Remove extraneous f prefix
(F541)
backend/tests/expenses/test_expense_routes.py
3-3: unittest.mock.AsyncMock imported but unused
Remove unused import: unittest.mock.AsyncMock
(F401)
5-5: app.expenses.schemas.ExpenseCreateRequest imported but unused
Remove unused import
(F401)
5-5: app.expenses.schemas.ExpenseSplit imported but unused
Remove unused import
(F401)
backend/app/expenses/routes.py
1-1: fastapi.Response imported but unused
Remove unused import: fastapi.Response
(F401)
2-2: fastapi.responses.StreamingResponse imported but unused
Remove unused import: fastapi.responses.StreamingResponse
(F401)
11-11: typing.List imported but unused
Remove unused import: typing.List
(F401)
12-12: datetime.timedelta imported but unused
Remove unused import: datetime.timedelta
(F401)
13-13: io imported but unused
Remove unused import: io
(F401)
24-24: Do not perform function call Depends in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
31-31: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
32-32: Local variable e is assigned to but never used
Remove assignment to unused variable e
(F841)
33-33: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
40-40: Do not perform function call Query in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
41-41: Do not perform function call Query in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
43-43: Do not perform function call Depends in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
53-53: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
54-54: Local variable e is assigned to but never used
Remove assignment to unused variable e
(F841)
55-55: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
61-61: Do not perform function call Depends in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
68-68: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
69-69: Local variable e is assigned to but never used
Remove assignment to unused variable e
(F841)
70-70: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
77-77: Do not perform function call Depends in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
84-84: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
89-89: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
95-95: Do not perform function call Depends in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
104-104: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
105-105: Local variable e is assigned to but never used
Remove assignment to unused variable e
(F841)
106-106: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
114-114: Do not perform function call File in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
115-115: Do not perform function call Depends in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
128-128: Local variable file_content is assigned to but never used
Remove assignment to unused variable file_content
(F841)
138-138: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
139-139: Local variable e is assigned to but never used
Remove assignment to unused variable e
(F841)
140-140: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
147-147: Do not perform function call Depends in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
158-158: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
159-159: Local variable e is assigned to but never used
Remove assignment to unused variable e
(F841)
160-160: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
168-168: Do not perform function call Depends in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
175-175: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
176-176: Local variable e is assigned to but never used
Remove assignment to unused variable e
(F841)
177-177: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
186-186: Do not perform function call Depends in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
223-223: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
224-224: Local variable e is assigned to but never used
Remove assignment to unused variable e
(F841)
225-225: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
231-231: Do not perform function call Depends in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
238-238: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
239-239: Local variable e is assigned to but never used
Remove assignment to unused variable e
(F841)
240-240: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
247-247: Do not perform function call Depends in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
256-256: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
257-257: Local variable e is assigned to but never used
Remove assignment to unused variable e
(F841)
258-258: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
264-264: Do not perform function call Depends in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
277-277: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
278-278: Local variable e is assigned to but never used
Remove assignment to unused variable e
(F841)
279-279: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
285-285: Do not perform function call Depends in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
309-309: Local variable e is assigned to but never used
Remove assignment to unused variable e
(F841)
310-310: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
319-319: Do not perform function call Depends in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
325-325: Local variable e is assigned to but never used
Remove assignment to unused variable e
(F841)
326-326: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
330-330: Do not perform function call Depends in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
336-336: Local variable e is assigned to but never used
Remove assignment to unused variable e
(F841)
337-337: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
344-344: Do not perform function call Depends in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
351-351: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
352-352: Local variable e is assigned to but never used
Remove assignment to unused variable e
(F841)
353-353: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
362-362: Do not perform function call Depends in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
369-369: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
370-370: Local variable e is assigned to but never used
Remove assignment to unused variable e
(F841)
371-371: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
backend/app/expenses/service.py
1-1: typing.Tuple imported but unused
Remove unused import: typing.Tuple
(F401)
7-7: app.expenses.schemas.SplitType imported but unused
Remove unused import: app.expenses.schemas.SplitType
(F401)
9-9: asyncio imported but unused
Remove unused import: asyncio
(F401)
10-10: collections.deque imported but unused
Remove unused import: collections.deque
(F401)
🪛 Flake8 (7.2.0)
backend/tests/expenses/test_expense_service.py
[error] 4-4: 'app.expenses.service.expense_service' imported but unused
(F401)
[error] 6-6: 'asyncio' imported but unused
(F401)
[error] 10-10: expected 2 blank lines, found 1
(E302)
[error] 14-14: local variable 'group_id' is assigned to but never used
(F841)
[error] 17-17: local variable 'settlements' is assigned to but never used
(F841)
[error] 29-29: expected 2 blank lines, found 1
(E302)
[error] 38-38: local variable 'user_balances' is assigned to but never used
(F841)
[error] 79-79: expected 2 blank lines, found 1
(E302)
[error] 110-110: expected 2 blank lines, found 1
(E302)
[error] 144-144: expected 2 blank lines after class or function definition, found 1
(E305)
backend/test_expense_service.py
[error] 8-8: 'json' imported but unused
(F401)
[error] 9-9: 'datetime.datetime' imported but unused
(F401)
[error] 13-13: expected 2 blank lines, found 1
(E302)
[error] 216-216: expected 2 blank lines after class or function definition, found 1
(E305)
backend/test_patch_endpoint.py
[error] 7-7: 'app.expenses.schemas.SplitType' imported but unused
(F401)
[error] 9-9: expected 2 blank lines, found 1
(E302)
[error] 54-54: local variable 'e' is assigned to but never used
(F841)
[error] 65-65: local variable 'update_request' is assigned to but never used
(F841)
[error] 72-72: expected 2 blank lines, found 1
(E302)
[error] 93-93: local variable 'mongodb_update' is assigned to but never used
(F841)
[error] 100-100: f-string is missing placeholders
(F541)
[error] 103-103: expected 2 blank lines after class or function definition, found 1
(E305)
backend/tests/expenses/test_expense_routes.py
[error] 3-3: 'unittest.mock.AsyncMock' imported but unused
(F401)
[error] 5-5: 'app.expenses.schemas.ExpenseCreateRequest' imported but unused
(F401)
[error] 5-5: 'app.expenses.schemas.ExpenseSplit' imported but unused
(F401)
[error] 9-9: expected 2 blank lines, found 1
(E302)
[error] 13-13: expected 2 blank lines, found 1
(E302)
[error] 27-27: expected 2 blank lines, found 1
(E302)
[error] 67-67: expected 2 blank lines, found 1
(E302)
[error] 98-98: expected 2 blank lines, found 1
(E302)
[error] 123-123: expected 2 blank lines, found 1
(E302)
[error] 146-146: expected 2 blank lines after class or function definition, found 1
(E305)
backend/app/expenses/routes.py
[error] 1-1: 'fastapi.Response' imported but unused
(F401)
[error] 2-2: 'fastapi.responses.StreamingResponse' imported but unused
(F401)
[error] 11-11: 'typing.List' imported but unused
(F401)
[error] 12-12: 'datetime.timedelta' imported but unused
(F401)
[error] 13-13: 'io' imported but unused
(F401)
[error] 20-20: expected 2 blank lines, found 1
(E302)
[error] 32-32: local variable 'e' is assigned to but never used
(F841)
[error] 35-35: expected 2 blank lines, found 1
(E302)
[error] 54-54: local variable 'e' is assigned to but never used
(F841)
[error] 57-57: expected 2 blank lines, found 1
(E302)
[error] 69-69: local variable 'e' is assigned to but never used
(F841)
[error] 72-72: expected 2 blank lines, found 1
(E302)
[error] 91-91: expected 2 blank lines, found 1
(E302)
[error] 105-105: local variable 'e' is assigned to but never used
(F841)
[error] 110-110: expected 2 blank lines, found 1
(E302)
[error] 128-128: local variable 'file_content' is assigned to but never used
(F841)
[error] 139-139: local variable 'e' is assigned to but never used
(F841)
[error] 142-142: expected 2 blank lines, found 1
(E302)
[error] 159-159: local variable 'e' is assigned to but never used
(F841)
[error] 164-164: expected 2 blank lines, found 1
(E302)
[error] 176-176: local variable 'e' is assigned to but never used
(F841)
[error] 179-179: expected 2 blank lines, found 1
(E302)
[error] 224-224: local variable 'e' is assigned to but never used
(F841)
[error] 227-227: expected 2 blank lines, found 1
(E302)
[error] 239-239: local variable 'e' is assigned to but never used
(F841)
[error] 242-242: expected 2 blank lines, found 1
(E302)
[error] 257-257: local variable 'e' is assigned to but never used
(F841)
[error] 260-260: expected 2 blank lines, found 1
(E302)
[error] 278-278: local variable 'e' is assigned to but never used
(F841)
[error] 281-281: expected 2 blank lines, found 1
(E302)
[error] 309-309: local variable 'e' is assigned to but never used
(F841)
[error] 317-317: expected 2 blank lines, found 1
(E302)
[error] 325-325: local variable 'e' is assigned to but never used
(F841)
[error] 328-328: expected 2 blank lines, found 1
(E302)
[error] 336-336: local variable 'e' is assigned to but never used
(F841)
[error] 340-340: expected 2 blank lines, found 1
(E302)
[error] 352-352: local variable 'e' is assigned to but never used
(F841)
[error] 356-356: expected 2 blank lines, found 1
(E302)
[error] 370-370: local variable 'e' is assigned to but never used
(F841)
backend/app/expenses/service.py
[error] 1-1: 'typing.Tuple' imported but unused
(F401)
[error] 5-5: 'app.expenses.schemas.SplitType' imported but unused
(F401)
[error] 9-9: 'asyncio' imported but unused
(F401)
[error] 10-10: 'collections.deque' imported but unused
(F401)
[error] 12-12: expected 2 blank lines, found 1
(E302)
[error] 119-119: continuation line under-indented for visual indent
(E128)
[error] 120-120: continuation line under-indented for visual indent
(E128)
[error] 537-537: continuation line under-indented for visual indent
(E128)
[error] 600-600: continuation line under-indented for visual indent
(E128)
[error] 943-943: continuation line under-indented for visual indent
(E128)
[error] 1050-1050: expected 2 blank lines after class or function definition, found 0
(E305)
backend/app/expenses/schemas.py
[error] 6-6: expected 2 blank lines, found 1
(E302)
[error] 11-11: expected 2 blank lines, found 1
(E302)
[error] 16-16: expected 2 blank lines, found 1
(E302)
[error] 21-21: expected 2 blank lines, found 1
(E302)
[error] 37-37: expected 2 blank lines, found 1
(E302)
[error] 57-57: expected 2 blank lines, found 1
(E302)
[error] 66-66: expected 2 blank lines, found 1
(E302)
[error] 75-75: expected 2 blank lines, found 1
(E302)
[error] 92-92: expected 2 blank lines, found 1
(E302)
[error] 108-108: expected 2 blank lines, found 1
(E302)
[error] 116-116: expected 2 blank lines, found 1
(E302)
[error] 121-121: expected 2 blank lines, found 1
(E302)
[error] 126-126: expected 2 blank lines, found 1
(E302)
[error] 131-131: expected 2 blank lines, found 1
(E302)
[error] 138-138: expected 2 blank lines, found 1
(E302)
[error] 142-142: expected 2 blank lines, found 1
(E302)
[error] 148-148: expected 2 blank lines, found 1
(E302)
[error] 158-158: expected 2 blank lines, found 1
(E302)
[error] 164-164: expected 2 blank lines, found 1
(E302)
[error] 173-173: expected 2 blank lines, found 1
(E302)
[error] 177-177: expected 2 blank lines, found 1
(E302)
[error] 184-184: expected 2 blank lines, found 1
(E302)
[error] 193-193: expected 2 blank lines, found 1
(E302)
[error] 197-197: expected 2 blank lines, found 1
(E302)
🪛 GitHub Actions: Run Tests
backend/tests/expenses/test_expense_service.py
[error] 1-1: ModuleNotFoundError: No module named 'app.main' when importing in test_expense_service.py
backend/tests/expenses/test_expense_routes.py
[error] 1-1: ModuleNotFoundError: No module named 'app.main' when importing in test_expense_routes.py
backend/app/expenses/schemas.py
[warning] 29-29: PydanticDeprecatedSince20: Pydantic V1 style '@validator' validators are deprecated. Migrate to '@field_validator'.
[warning] 44-44: PydanticDeprecatedSince20: Pydantic V1 style '@validator' validators are deprecated. Migrate to '@field_validator'.
🪛 Pylint (3.3.7)
backend/test_expense_service.py
[refactor] 147-147: Too many local variables (16/15)
(R0914)
[refactor] 13-13: Too many statements (92/50)
(R0915)
backend/app/expenses/routes.py
[refactor] 36-36: Too many arguments (7/5)
(R0913)
[refactor] 36-36: Too many positional arguments (7/5)
(R0917)
[refactor] 180-180: Too many arguments (6/5)
(R0913)
[refactor] 180-180: Too many positional arguments (6/5)
(R0917)
backend/app/expenses/service.py
[refactor] 118-118: Too many arguments (8/5)
(R0913)
[refactor] 118-118: Too many positional arguments (8/5)
(R0917)
[refactor] 118-118: Too many local variables (21/15)
(R0914)
[refactor] 335-338: Unnecessary "else" after "return", remove the "else" and de-indent the code inside it
(R1705)
[refactor] 392-392: Too many local variables (21/15)
(R0914)
[refactor] 536-536: Too many arguments (6/5)
(R0913)
[refactor] 536-536: Too many positional arguments (6/5)
(R0917)
[refactor] 599-599: Too many arguments (6/5)
(R0913)
[refactor] 599-599: Too many positional arguments (6/5)
(R0917)
[refactor] 645-645: Too many local variables (22/15)
(R0914)
[refactor] 750-750: Too many local variables (19/15)
(R0914)
[refactor] 942-942: Too many arguments (6/5)
(R0913)
[refactor] 942-942: Too many positional arguments (6/5)
(R0917)
[refactor] 942-942: Too many local variables (31/15)
(R0914)
[refactor] 942-942: Too many branches (16/12)
(R0912)
backend/app/expenses/schemas.py
[refactor] 16-16: Too few public methods (0/2)
(R0903)
[error] 30-30: Method 'validate_splits_sum' should have "self" as first argument
(E0213)
[refactor] 21-21: Too few public methods (1/2)
(R0903)
[error] 45-45: Method 'validate_splits_sum' should have "self" as first argument
(E0213)
[refactor] 53-53: Too few public methods (0/2)
(R0903)
[refactor] 37-37: Too few public methods (1/2)
(R0903)
[refactor] 57-57: Too few public methods (0/2)
(R0903)
[refactor] 66-66: Too few public methods (0/2)
(R0903)
[refactor] 75-75: Too few public methods (0/2)
(R0903)
[refactor] 92-92: Too few public methods (0/2)
(R0903)
[refactor] 108-108: Too few public methods (0/2)
(R0903)
[refactor] 116-116: Too few public methods (0/2)
(R0903)
[refactor] 121-121: Too few public methods (0/2)
(R0903)
[refactor] 126-126: Too few public methods (0/2)
(R0903)
[refactor] 131-131: Too few public methods (0/2)
(R0903)
[refactor] 138-138: Too few public methods (0/2)
(R0903)
[refactor] 142-142: Too few public methods (0/2)
(R0903)
[refactor] 148-148: Too few public methods (0/2)
(R0903)
[refactor] 158-158: Too few public methods (0/2)
(R0903)
[refactor] 164-164: Too few public methods (0/2)
(R0903)
[refactor] 173-173: Too few public methods (0/2)
(R0903)
[refactor] 177-177: Too few public methods (0/2)
(R0903)
[refactor] 184-184: Too few public methods (0/2)
(R0903)
[refactor] 193-193: Too few public methods (0/2)
(R0903)
[refactor] 197-197: Too few public methods (0/2)
(R0903)
🪛 LanguageTool
backend/app/expenses/README.md
[uncategorized] ~142-~142: You might be missing the article “the” here.
Context: ...tion Rules - Split amounts must sum to total expense amount (±0.01 tolerance) - All ...
(AI_EN_LECTOR_MISSING_DETERMINER_THE)
[uncategorized] ~149-~149: Loose punctuation mark.
Context: ... ## Error Handling - 400 Bad Request: Invalid expense data or splits - `401 U...
(UNLIKELY_OPENING_PUNCTUATION)
backend/PATCH_FIX_SUMMARY.md
[uncategorized] ~6-~6: You might be missing the article “a” here.
Context: ... $push inside $set operation caused MongoDB error. Fix: Separated $set and `$...
(AI_EN_LECTOR_MISSING_DETERMINER_A)
[style] ~72-~72: It’s more common nowadays to write this noun as one word.
Context: ...n - Graceful settlement recalculation - User name fallback handling ### 6. Created Debug...
(RECOMMENDED_COMPOUNDS)
[grammar] ~114-~114: You’ve repeated a verb. Did you mean to only write one of them?
Context: ...patch_endpoint.py**: Created validation tests 5. test_expense_service.py: Added PATCH testi...
(REPEATED_VERBS)
🪛 markdownlint-cli2 (0.17.2)
backend/app/expenses/README.md
53-53: Fenced code blocks should have a language specified
null
(MD040, fenced-code-language)
62-62: Fenced code blocks should have a language specified
null
(MD040, fenced-code-language)
68-68: Fenced code blocks should have a language specified
null
(MD040, fenced-code-language)
78-78: Fenced code blocks should have a language specified
null
(MD040, fenced-code-language)
🔇 Additional comments (19)
backend/main.py (2)
9-9: LGTM! Router import follows established pattern.The import statement correctly imports both routers from the expenses module following the existing convention.
108-109: LGTM! Router integration is properly implemented.Both routers are correctly included in the FastAPI application, maintaining consistency with the existing router integration pattern.
backend/tests/expenses/test_expense_service.py (3)
29-78: LGTM! Advanced settlement algorithm test is well-structured.The two-pointer algorithm implementation and test validation correctly verify the optimization logic, ensuring that the advanced algorithm reduces transaction count.
79-109: LGTM! Expense split validation test covers key scenarios.The test properly validates that split amounts must sum to the total expense amount and correctly expects a ValueError for invalid splits.
110-143: LGTM! Split type testing is comprehensive.The test covers both equal and unequal split types with proper validation of the split type enumeration.
backend/test_patch_endpoint.py (2)
9-71: LGTM! PATCH validation testing is comprehensive.The test function covers all important PATCH scenarios including partial updates, validation errors, and edge cases. The validation logic correctly handles different update combinations.
72-102: LGTM! MongoDB update structure test is valuable.The test demonstrates the correct separation of
$setand$pushoperations in MongoDB updates, which is crucial for avoiding update conflicts.backend/test_expense_service.py (4)
13-58: LGTM! Comprehensive API endpoint documentation.The function provides excellent documentation of available endpoints and helpful testing instructions for developers.
59-109: LGTM! Split validation testing is thorough.The validation logic and test cases cover various scenarios including valid equal/unequal splits and invalid cases that don't sum correctly.
110-212: LGTM! Settlement algorithm implementations are well-designed.Both normal and advanced settlement algorithms are correctly implemented with proper logic for optimizing transactions. The comparison between algorithms effectively demonstrates the optimization benefits.
213-249: LGTM! Debugging guidance is valuable.The detailed PATCH endpoint testing instructions and common error causes provide excellent debugging support for developers.
backend/tests/expenses/test_expense_routes.py (5)
9-26: LGTM! Test fixtures are well-structured.The mock user and sample expense data fixtures provide good test data setup for the route tests.
27-66: LGTM! Create expense endpoint test is comprehensive.The test properly mocks dependencies and validates the expected response structure. The flexible assertion for status codes accounts for different authentication scenarios.
67-97: LGTM! List expenses endpoint test covers pagination and summary.The test mocks the service response with proper pagination structure and validates the endpoint behavior correctly.
98-122: LGTM! Optimized settlements test validates the optimization endpoint.The test properly mocks the settlement calculation and verifies the endpoint structure with realistic settlement data.
123-145: LGTM! Validation test covers important error scenario.The test correctly validates that invalid split data (amounts not summing to total) returns appropriate error status codes.
backend/app/expenses/service.py (2)
227-348: Excellent error handling and validation in update_expense method.The implementation properly handles:
- ObjectId validation
- Partial update validation
- MongoDB operation separation ($set and $push)
- Comprehensive error logging
- Settlement recalculation
Great work on the PATCH endpoint fixes!
428-500: Well-implemented advanced settlement algorithm.The graph-based optimization algorithm correctly:
- Calculates net balances for each user
- Separates debtors and creditors
- Uses two-pointer technique for optimal matching
- Handles floating-point precision properly
This implementation efficiently minimizes the number of transactions.
backend/app/expenses/schemas.py (1)
44-55: Well-implemented validation for partial updates.The validator correctly handles partial updates by only validating when both
splitsandamountare provided. This fix properly addresses the PATCH endpoint issues.
| @@ -0,0 +1,1086 @@ | |||
| from typing import List, Dict, Any, Optional, Tuple | |||
There was a problem hiding this comment.
Remove unused imports.
Several imports are not used in the code and should be removed.
-from typing import List, Dict, Any, Optional, Tuple
+from typing import List, Dict, Any, Optional
from datetime import datetime, timedelta
from bson import ObjectId
from app.database import mongodb
from app.expenses.schemas import (
- ExpenseCreateRequest, ExpenseUpdateRequest, ExpenseResponse, Settlement,
- OptimizedSettlement, SettlementCreateRequest, SettlementStatus, SplitType
+ ExpenseCreateRequest, ExpenseUpdateRequest, ExpenseResponse, Settlement,
+ OptimizedSettlement, SettlementCreateRequest, SettlementStatus
)
-import asyncio
-from collections import defaultdict, deque
+from collections import defaultdictAlso applies to: 5-10
🧰 Tools
🪛 Flake8 (7.2.0)
[error] 1-1: 'typing.Tuple' imported but unused
(F401)
🪛 Ruff (0.11.9)
1-1: typing.Tuple imported but unused
Remove unused import: typing.Tuple
(F401)
🤖 Prompt for AI Agents
In backend/app/expenses/service.py from lines 1 to 10, several imports such as
List, Dict, Any, Optional, and Tuple are not used in the code. Review the
imports in these lines and remove any that are not referenced anywhere in the
file to clean up and optimize the code.
| except ValueError as e: | ||
| raise HTTPException(status_code=400, detail=str(e)) | ||
| except Exception as e: | ||
| raise HTTPException(status_code=500, detail="Failed to create expense") |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve exception handling with proper chaining.
Exception handling should use from e for proper exception chaining and avoid unused variables.
Example fix for lines 30-33:
except ValueError as e:
raise HTTPException(status_code=400, detail=str(e))
- except Exception as e:
- raise HTTPException(status_code=500, detail="Failed to create expense")
+ except Exception as e:
+ raise HTTPException(status_code=500, detail="Failed to create expense") from eOr if not using the exception:
- except Exception as e:
+ except Exception:
raise HTTPException(status_code=500, detail="Failed to create expense")Apply similar fixes throughout the file for all exception handlers.
Also applies to: 52-55, 67-70
🧰 Tools
🪛 Flake8 (7.2.0)
[error] 32-32: local variable 'e' is assigned to but never used
(F841)
🪛 Ruff (0.11.9)
31-31: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
32-32: Local variable e is assigned to but never used
Remove assignment to unused variable e
(F841)
33-33: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
🤖 Prompt for AI Agents
In backend/app/expenses/routes.py at lines 30-33, 52-55, and 67-70, the
exception handlers catch exceptions but do not use proper exception chaining
with 'from e', and the caught exception variables are unused. To fix this,
modify each 'raise HTTPException' statement to include 'from e' after the
exception to maintain the original traceback, and if the exception variable is
not used, remove it to avoid unused variables. Apply this pattern consistently
to all exception handlers in these line ranges.
| from fastapi import APIRouter, Depends, HTTPException, status, Query, UploadFile, File, Response | ||
| from fastapi.responses import StreamingResponse |
There was a problem hiding this comment.
Remove unused imports.
Several imports are not used in the code.
-from fastapi import APIRouter, Depends, HTTPException, status, Query, UploadFile, File, Response
+from fastapi import APIRouter, Depends, HTTPException, status, Query, UploadFile, File
-from fastapi.responses import StreamingResponse
from app.expenses.schemas import (
ExpenseCreateRequest, ExpenseCreateResponse, ExpenseListResponse, ExpenseResponse,
ExpenseUpdateRequest, SettlementCreateRequest, Settlement, SettlementUpdateRequest,
SettlementListResponse, OptimizedSettlementsResponse, FriendsBalanceResponse,
BalanceSummaryResponse, UserBalance, ExpenseAnalytics, AttachmentUploadResponse
)
from app.expenses.service import expense_service
from app.auth.security import get_current_user
-from typing import Dict, Any, List, Optional
+from typing import Dict, Any, Optional
-from datetime import datetime, timedelta
+from datetime import datetime
-import io
import uuidAlso applies to: 11-14
🧰 Tools
🪛 Flake8 (7.2.0)
[error] 1-1: 'fastapi.Response' imported but unused
(F401)
[error] 2-2: 'fastapi.responses.StreamingResponse' imported but unused
(F401)
🪛 Ruff (0.11.9)
1-1: fastapi.Response imported but unused
Remove unused import: fastapi.Response
(F401)
2-2: fastapi.responses.StreamingResponse imported but unused
Remove unused import: fastapi.responses.StreamingResponse
(F401)
🤖 Prompt for AI Agents
In backend/app/expenses/routes.py around lines 1 to 14, some imports such as
Depends, HTTPException, status, Query, UploadFile, File, Response, and
StreamingResponse are not used anywhere in the code. Remove these unused imports
to clean up the code and avoid unnecessary dependencies.
| @validator('splits') | ||
| def validate_splits_sum(cls, v, values): | ||
| if 'amount' in values: | ||
| total_split = sum(split.amount for split in v) | ||
| if abs(total_split - values['amount']) > 0.01: # Allow small floating point differences | ||
| raise ValueError('Split amounts must sum to total expense amount') | ||
| return v |
There was a problem hiding this comment.
Migrate to Pydantic v2 field validators.
The @validator decorator is deprecated in Pydantic v2. Migrate to @field_validator.
-from pydantic import BaseModel, Field, validator
+from pydantic import BaseModel, Field, field_validator
class ExpenseCreateRequest(BaseModel):
# ... fields ...
- @validator('splits')
- def validate_splits_sum(cls, v, values):
- if 'amount' in values:
+ @field_validator('splits')
+ @classmethod
+ def validate_splits_sum(cls, v, info):
+ if 'amount' in info.data:
- total_split = sum(split.amount for split in v)
- if abs(total_split - values['amount']) > 0.01:
+ total_split = sum(split.amount for split in v)
+ if abs(total_split - info.data['amount']) > 0.01:
raise ValueError('Split amounts must sum to total expense amount')
return vApply similar changes to the validator in ExpenseUpdateRequest.
Also applies to: 44-51
🧰 Tools
🪛 Pylint (3.3.7)
[error] 30-30: Method 'validate_splits_sum' should have "self" as first argument
(E0213)
🪛 GitHub Actions: Run Tests
[warning] 29-29: PydanticDeprecatedSince20: Pydantic V1 style '@validator' validators are deprecated. Migrate to '@field_validator'.
🤖 Prompt for AI Agents
In backend/app/expenses/schemas.py around lines 29 to 35 and 44 to 51, the code
uses the deprecated @validator decorator from Pydantic v1. Replace @validator
with @field_validator according to Pydantic v2 syntax. Adjust the validator
methods to use the new signature and return style required by @field_validator,
ensuring the validation logic remains the same. Apply these changes to both the
validate_splits_sum method and the corresponding validator in
ExpenseUpdateRequest.
* Fix expense test imports and CI test command - Corrected `ModuleNotFoundError` in expense tests by changing `from app.main import app` to `from main import app`. This aligns with the project structure and how group tests perform their imports. - Updated the GitHub Actions workflow (`run-tests.yml`) to use `python -m pytest` instead of just `pytest`. This resolves a `pytest-asyncio` plugin discovery issue encountered during testing, ensuring CI runs the tests with the correct Python environment context. * Update code structure for improved readability and maintainability --------- Co-authored-by: google-labs-jules[bot] <161369871+google-labs-jules[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
backend/tests/expenses/test_expense_service.py (1)
9-27: Complete the placeholder test implementation.This test still contains placeholder logic and unused variables, which was flagged in a previous review. The settlement algorithm validation needs actual implementation.
Replace the placeholder with actual validation logic:
@pytest.mark.asyncio async def test_settlement_algorithm_normal(): """Test normal settlement algorithm""" - # Mock data for testing - group_id = "test_group_123" - - # Create some mock settlements - settlements = [ - {"payerId": "user_a", "payeeId": "user_b", "amount": 100, "payerName": "Alice", "payeeName": "Bob"}, - {"payerId": "user_b", "payeeId": "user_a", "amount": 50, "payerName": "Bob", "payeeName": "Alice"}, - {"payerId": "user_a", "payeeId": "user_c", "amount": 75, "payerName": "Alice", "payeeName": "Charlie"}, - ] - - # This would need to be adapted to work with the actual database - # For now, test the algorithm logic conceptually - - # Expected: Alice owes Bob 50 (100-50), Alice is owed 75 by Charlie - assert True # Placeholder assertion + # Test basic settlement calculation + settlements = [ + {"payerId": "user_a", "payeeId": "user_b", "amount": 100}, + {"payerId": "user_b", "payeeId": "user_a", "amount": 50}, + {"payerId": "user_a", "payeeId": "user_c", "amount": 75}, + ] + + # Calculate net settlements + net_balances = {} + for settlement in settlements: + payer = settlement["payerId"] + payee = settlement["payeeId"] + amount = settlement["amount"] + + net_balances[payer] = net_balances.get(payer, 0) + amount + net_balances[payee] = net_balances.get(payee, 0) - amount + + # Verify expected net balances + assert net_balances["user_a"] == 125 # owes 175, owed 50 + assert net_balances["user_b"] == -50 # owes 50, owed 100 + assert net_balances["user_c"] == -75 # owed 75backend/tests/expenses/test_expense_routes.py (1)
1-7: Clean up unused imports and fix formatting.Remove unused imports and fix inline comment formatting as flagged by static analysis.
import pytest from httpx import AsyncClient, ASGITransport from fastapi import status -from unittest.mock import AsyncMock, patch -from main import app # Adjusted import -from app.expenses.schemas import ExpenseCreateRequest, ExpenseSplit +from unittest.mock import patch +from main import app # Adjusted import
🧹 Nitpick comments (4)
backend/tests/expenses/test_expense_service.py (2)
1-6: Clean up unused imports.Several imports are unused according to static analysis. Remove them to improve code cleanliness.
import pytest -from main import app # Adjusted import - Keep app import for context if needed, but TestClient is removed -from app.expenses.service import expense_service from app.expenses.schemas import ExpenseCreateRequest, ExpenseSplit, SplitType -import asyncio
37-37: Remove unused variable.The
user_balancesvariable is declared but never used in the test logic.- user_balances = { - "user_a": 100, # A owes $100 - "user_b": 0, # B is neutral (owes 100, owed 100) - "user_c": -100 # C is owed $100 - } - # Simulate the advanced algorithm logicbackend/tests/expenses/test_expense_routes.py (2)
34-71: Enhance test assertion specificity.While the flexible status code assertion acknowledges authentication challenges, consider making the test more deterministic by properly mocking the authentication dependency.
@pytest.mark.asyncio -@patch("app.expenses.routes.get_current_user") +@patch("app.dependencies.get_current_user") # Adjust path if needed @patch("app.expenses.service.expense_service.create_expense") async def test_create_expense_endpoint(mock_create_expense, mock_get_current_user, sample_expense_data, mock_current_user, async_client: AsyncClient): """Test create expense endpoint""" mock_get_current_user.return_value = mock_current_user mock_create_expense.return_value = { # ... existing mock data } - response = await async_client.post( - "/groups/group_123/expenses", - json=sample_expense_data, - headers={"Authorization": "Bearer test_token"} - ) + # Test without authentication dependency + with patch("app.expenses.routes.Depends") as mock_depends: + mock_depends.return_value = mock_current_user + response = await async_client.post( + "/groups/group_123/expenses", + json=sample_expense_data + ) - # This test would need proper authentication mocking to work - # For now, it demonstrates the structure - assert response.status_code in [status.HTTP_201_CREATED, status.HTTP_401_UNAUTHORIZED, status.HTTP_422_UNPROCESSABLE_ENTITY] # Depending on auth setup + assert response.status_code == status.HTTP_201_CREATED + mock_create_expense.assert_called_once()
72-103: Similar authentication mocking improvement needed.This test has the same authentication challenges as the create expense test. Consider applying similar dependency injection mocking.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
backend/tests/expenses/test_expense_routes.py(1 hunks)backend/tests/expenses/test_expense_service.py(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (2)
backend/tests/expenses/test_expense_routes.py (1)
backend/app/expenses/schemas.py (2)
ExpenseCreateRequest(21-35)ExpenseSplit(16-19)
backend/tests/expenses/test_expense_service.py (1)
backend/app/expenses/schemas.py (3)
ExpenseCreateRequest(21-35)ExpenseSplit(16-19)SplitType(6-9)
🪛 Ruff (0.11.9)
backend/tests/expenses/test_expense_routes.py
4-4: unittest.mock.AsyncMock imported but unused
Remove unused import: unittest.mock.AsyncMock
(F401)
6-6: app.expenses.schemas.ExpenseCreateRequest imported but unused
Remove unused import
(F401)
6-6: app.expenses.schemas.ExpenseSplit imported but unused
Remove unused import
(F401)
backend/tests/expenses/test_expense_service.py
2-2: main.app imported but unused
Remove unused import: main.app
(F401)
3-3: app.expenses.service.expense_service imported but unused
Remove unused import: app.expenses.service.expense_service
(F401)
5-5: asyncio imported but unused
Remove unused import: asyncio
(F401)
13-13: Local variable group_id is assigned to but never used
Remove assignment to unused variable group_id
(F841)
16-16: Local variable settlements is assigned to but never used
Remove assignment to unused variable settlements
(F841)
37-37: Local variable user_balances is assigned to but never used
Remove assignment to unused variable user_balances
(F841)
🪛 Flake8 (7.2.0)
backend/tests/expenses/test_expense_routes.py
[error] 4-4: 'unittest.mock.AsyncMock' imported but unused
(F401)
[error] 5-5: at least two spaces before inline comment
(E261)
[error] 6-6: 'app.expenses.schemas.ExpenseCreateRequest' imported but unused
(F401)
[error] 6-6: 'app.expenses.schemas.ExpenseSplit' imported but unused
(F401)
[error] 8-8: expected 2 blank lines, found 1
(E302)
[error] 13-13: expected 2 blank lines, found 1
(E302)
[error] 17-17: expected 2 blank lines, found 1
(E302)
[error] 31-31: expected 2 blank lines, found 1
(E302)
[error] 72-72: expected 2 blank lines, found 1
(E302)
[error] 104-104: expected 2 blank lines, found 1
(E302)
[error] 130-130: expected 2 blank lines, found 1
(E302)
backend/tests/expenses/test_expense_service.py
[error] 2-2: 'main.app' imported but unused
(F401)
[error] 2-2: at least two spaces before inline comment
(E261)
[error] 3-3: 'app.expenses.service.expense_service' imported but unused
(F401)
[error] 5-5: 'asyncio' imported but unused
(F401)
[error] 9-9: expected 2 blank lines, found 1
(E302)
[error] 13-13: local variable 'group_id' is assigned to but never used
(F841)
[error] 16-16: local variable 'settlements' is assigned to but never used
(F841)
[error] 28-28: expected 2 blank lines, found 1
(E302)
[error] 37-37: local variable 'user_balances' is assigned to but never used
(F841)
[error] 78-78: expected 2 blank lines, found 1
(E302)
[error] 109-109: expected 2 blank lines, found 1
(E302)
[error] 143-143: expected 2 blank lines after class or function definition, found 1
(E305)
🔇 Additional comments (6)
backend/tests/expenses/test_expense_service.py (3)
28-77: Excellent implementation of the advanced settlement algorithm test.This test effectively demonstrates the two-pointer optimization algorithm, showing how it reduces transactions from 2 to 1 by eliminating intermediate settlements. The test logic is clear and validates the algorithm correctly.
78-108: Well-implemented validation test.The test correctly validates both positive and negative cases for expense split validation, ensuring splits sum to the total amount within the tolerance specified in the schema.
109-142: Comprehensive split type testing.Good coverage of different split types (equal and unequal) with proper validation of the splitType field and amount calculations.
backend/tests/expenses/test_expense_routes.py (3)
8-30: Well-structured test fixtures.The fixtures provide good mock data and setup for testing. The
sample_expense_datafixture properly matches the expected schema format.
104-129: Good settlement optimization test structure.The test properly mocks the settlement calculation and validates the expected endpoint behavior. The mock return data structure looks comprehensive.
130-153: Effective validation testing.This test correctly validates the API's handling of invalid expense data where splits don't sum to the total amount, which aligns with the schema validation rules.
|
@codecov-ai-reviewer test |
|
On it! Codecov is generating unit tests for this PR. |
|
Sentry has determined that unit tests are not necessary for this PR. |
|
@codecov-ai-reviewer review |
|
On it! We are reviewing the PR and will provide feedback shortly. |
PR DescriptionThis pull request introduces a comprehensive Expense Service to the backend, enabling users to create, manage, and track expenses within groups. It also includes settlement calculation and optimization features to simplify debt resolution among group members. Click to see moreKey Technical ChangesKey changes include: Architecture DecisionsNotable architectural decisions include: Dependencies and InteractionsThe Expense Service depends on: Risk ConsiderationsPotential risks and considerations include: Notable Implementation DetailsNotable implementation details include: |
![codecov-ai[bot]](https://avatars.githubusercontent.com/in/797565?s=60&v=4){kind=small}
| # Get updated expense | ||
| updated_expense = await self.expenses_collection.find_one({"_id": expense_obj_id}) | ||
|
|
||
| if updated_expense: | ||
| # Create new settlements | ||
| await self._create_settlements_for_expense(updated_expense, user_id) | ||
| except Exception as e: |
There was a problem hiding this comment.
The error handling in the update_expense method catches all exceptions with a generic handler, which can mask important database errors. Consider being more specific about which exceptions to catch and how to handle them. For example, separate handling for validation errors, database connection errors, and ObjectId conversion errors.
| # Get updated expense | |
| updated_expense = await self.expenses_collection.find_one({"_id": expense_obj_id}) | |
| if updated_expense: | |
| # Create new settlements | |
| await self._create_settlements_for_expense(updated_expense, user_id) | |
| except Exception as e: | |
| except ValueError: | |
| raise | |
| except ObjectIdError as e: | |
| raise ValueError(f"Invalid expense ID format: {expense_id}") | |
| except PyMongoError as e: | |
| print(f"Database error in update_expense: {str(e)}") | |
| import traceback | |
| traceback.print_exc() | |
| raise Exception(f"Database error during expense update: {str(e)}") | |
| except Exception as e: | |
| print(f"Unexpected error in update_expense: {str(e)}") | |
| import traceback | |
| traceback.print_exc() | |
| raise Exception(f"Unexpected error during expense update: {str(e)}") |
| await self.settlements_collection.delete_many({"expenseId": expense_id}) | ||
|
|
||
| # Get updated expense | ||
| updated_expense = await self.expenses_collection.find_one({"_id": expense_obj_id}) |
There was a problem hiding this comment.
Logging to stdout with print() is not production-ready. Consider using Python's logging module with proper log levels and structured logging for better observability.
| await self.settlements_collection.delete_many({"expenseId": expense_id}) | |
| # Get updated expense | |
| updated_expense = await self.expenses_collection.find_one({"_id": expense_obj_id}) | |
| import logging | |
| logger = logging.getLogger(__name__) | |
| # Then replace print statements with: | |
| logger.error(f"Error in update_expense: {str(e)}") | |
| logger.error(f"Traceback: {traceback.format_exc()}") |
There was a problem hiding this comment.
@Devasy23 create ticket for logger module
| "createdBy": user_id | ||
| }) | ||
| if not expense_doc: | ||
| raise ValueError("Expense not found or not authorized to edit") | ||
|
|
||
| # Validate splits against current or new amount if both are being updated | ||
| if updates.splits is not None and updates.amount is not None: | ||
| total_split = sum(split.amount for split in updates.splits) | ||
| if abs(total_split - updates.amount) > 0.01: |
There was a problem hiding this comment.
The split validation logic is duplicated in the schemas.py validator and the service layer. Consider centralizing this validation logic in a utility function to avoid code duplication and ensure consistency.
| "createdBy": user_id | |
| }) | |
| if not expense_doc: | |
| raise ValueError("Expense not found or not authorized to edit") | |
| # Validate splits against current or new amount if both are being updated | |
| if updates.splits is not None and updates.amount is not None: | |
| total_split = sum(split.amount for split in updates.splits) | |
| if abs(total_split - updates.amount) > 0.01: | |
| # Create a utility function in a utils.py file: | |
| def validate_splits_against_amount(splits, amount, tolerance=0.01): | |
| """Validate that splits sum to the total amount within tolerance""" | |
| total_split = sum(split.amount for split in splits) | |
| if abs(total_split - amount) > tolerance: | |
| raise ValueError('Split amounts must sum to total expense amount') | |
| return True | |
| # Then use it in the service: | |
| if updates.splits is not None and updates.amount is not None: | |
| validate_splits_against_amount(updates.splits, updates.amount) |
| @validator('splits') | ||
| def validate_splits_sum(cls, v, values): | ||
| # Only validate if both splits and amount are provided in the update | ||
| if v is not None and 'amount' in values and values['amount'] is not None: | ||
| total_split = sum(split.amount for split in v) | ||
| if abs(total_split - values['amount']) > 0.01: | ||
| raise ValueError('Split amounts must sum to total expense amount') |
There was a problem hiding this comment.
The validator for splits validation has a potential issue where it only validates when both fields are present, but doesn't handle the case where splits are updated without amount. The service layer handles this, but having inconsistent validation between schema and service can be confusing.
| @validator('splits') | |
| def validate_splits_sum(cls, v, values): | |
| # Only validate if both splits and amount are provided in the update | |
| if v is not None and 'amount' in values and values['amount'] is not None: | |
| total_split = sum(split.amount for split in v) | |
| if abs(total_split - values['amount']) > 0.01: | |
| raise ValueError('Split amounts must sum to total expense amount') | |
| @validator('splits') | |
| def validate_splits_sum(cls, v, values): | |
| # Only validate when both splits and amount are provided in the same request | |
| # Server-side validation in service layer handles splits-only updates | |
| if v is not None and 'amount' in values and values['amount'] is not None: | |
| total_split = sum(split.amount for split in v) | |
| if abs(total_split - values['amount']) > 0.01: | |
| raise ValueError('Split amounts must sum to total expense amount') | |
| return v |
| expense_id = str(expense_doc["_id"]) | ||
| group_id = expense_doc["groupId"] | ||
|
|
||
| # Get user names for the settlements | ||
| user_ids = [split["userId"] for split in expense_doc["splits"]] + [payer_id] |
There was a problem hiding this comment.
The _create_settlements_for_expense method could potentially fail if user lookup fails, but there's no error handling for missing users. This could cause the entire expense creation to fail silently.
| expense_id = str(expense_doc["_id"]) | |
| group_id = expense_doc["groupId"] | |
| # Get user names for the settlements | |
| user_ids = [split["userId"] for split in expense_doc["splits"]] + [payer_id] | |
| # Get user names for the settlements | |
| user_ids = [split["userId"] for split in expense_doc["splits"]] + [payer_id] | |
| try: | |
| users = await self.users_collection.find({"_id": {"$in": [ObjectId(uid) for uid in user_ids]}}).to_list(None) | |
| except Exception as e: | |
| logger.error(f"Failed to fetch user data for settlements: {e}") | |
| # Fallback to unknown names rather than failing | |
| users = [] | |
| user_names = {str(user["_id"]): user.get("name", "Unknown") for user in users} | |
| # Ensure all users have names, even if not found in database | |
| for user_id in user_ids: | |
| if user_id not in user_names: | |
| user_names[user_id] = "Unknown User" |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #23 +/- ##
==========================================
+ Coverage 81.56% 84.09% +2.52%
==========================================
Files 22 27 +5
Lines 1427 3176 +1749
==========================================
+ Hits 1164 2671 +1507
- Misses 263 505 +242
🚀 New features to boost your workflow:
|
…enarios and debugging
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
backend/test_expense_service.py (3)
8-9: Remove unused imports.The
jsonanddatetimeimports are not used anywhere in the script.-import json -from datetime import datetime
205-208: Remove unnecessary f-string prefixes.These strings don't contain any placeholders, so the
fprefix is unnecessary.- print(f" Test scenario:") - print(f" Alice paid for Bob: Bob owes Alice $100") - print(f" Bob paid for Charlie: Charlie owes Bob $100") - print(f" Expected optimization: Charlie pays Alice $100 directly") + print(" Test scenario:") + print(" Alice paid for Bob: Bob owes Alice $100") + print(" Bob paid for Charlie: Charlie owes Bob $100") + print(" Expected optimization: Charlie pays Alice $100 directly") - print(f"\n🔍 Advanced Algorithm Debug:") + print("\n🔍 Advanced Algorithm Debug:")Also applies to: 223-223
13-13: Fix PEP 8 formatting issues.Add the required blank lines for proper function spacing.
+ def test_expense_apis(): if __name__ == "__main__": test_expense_apis() +Also applies to: 313-313
backend/EXPENSE_SERVICE_COMPLETION_SUMMARY.md (1)
49-49: Improve documentation formatting.Address the following minor formatting issues:
-``` +```text ⚖️ Settlement Algorithm Test Results: -2. **Access API docs**: http://localhost:8000/docs +2. **Access API docs**: <http://localhost:8000/docs>Also applies to: 130-130
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
backend/EXPENSE_SERVICE_COMPLETION_SUMMARY.md(1 hunks)backend/test_expense_service.py(1 hunks)
🧰 Additional context used
🪛 LanguageTool
backend/EXPENSE_SERVICE_COMPLETION_SUMMARY.md
[grammar] ~100-~100: Using ‘plenty’ without ‘of’ is considered to be informal.
Context: ...validation that splits sum equals total amount - Support for equal and unequal split types - Com...
(PLENTY_OF_NOUNS)
🪛 markdownlint-cli2 (0.17.2)
backend/EXPENSE_SERVICE_COMPLETION_SUMMARY.md
49-49: Fenced code blocks should have a language specified
null
(MD040, fenced-code-language)
130-130: Bare URL used
null
(MD034, no-bare-urls)
🪛 Ruff (0.11.9)
backend/test_expense_service.py
8-8: json imported but unused
Remove unused import: json
(F401)
9-9: datetime.datetime imported but unused
Remove unused import: datetime.datetime
(F401)
205-205: f-string without any placeholders
Remove extraneous f prefix
(F541)
206-206: f-string without any placeholders
Remove extraneous f prefix
(F541)
207-207: f-string without any placeholders
Remove extraneous f prefix
(F541)
208-208: f-string without any placeholders
Remove extraneous f prefix
(F541)
223-223: f-string without any placeholders
Remove extraneous f prefix
(F541)
🪛 Flake8 (7.2.0)
backend/test_expense_service.py
[error] 8-8: 'json' imported but unused
(F401)
[error] 9-9: 'datetime.datetime' imported but unused
(F401)
[error] 13-13: expected 2 blank lines, found 1
(E302)
[error] 205-205: f-string is missing placeholders
(F541)
[error] 206-206: f-string is missing placeholders
(F541)
[error] 207-207: f-string is missing placeholders
(F541)
[error] 208-208: f-string is missing placeholders
(F541)
[error] 223-223: f-string is missing placeholders
(F541)
[error] 313-313: expected 2 blank lines after class or function definition, found 1
(E305)
🪛 Pylint (3.3.7)
backend/test_expense_service.py
[refactor] 13-13: Too many local variables (27/15)
(R0914)
[refactor] 147-147: Too many local variables (16/15)
(R0914)
[refactor] 13-13: Too many branches (13/12)
(R0912)
[refactor] 13-13: Too many statements (159/50)
(R0915)
🔇 Additional comments (3)
backend/test_expense_service.py (2)
62-108: Excellent split validation implementation.The split validation logic correctly handles floating-point arithmetic with appropriate tolerance (0.01) and includes comprehensive test cases covering both valid and invalid scenarios.
113-194: Well-implemented settlement algorithms.Both the normal and advanced settlement algorithms are mathematically sound:
- Normal algorithm correctly nets out reciprocal transactions
- Advanced algorithm uses an optimal greedy approach with two-pointer matching to minimize total transactions
The implementations match the expected algorithmic behavior described in the PR objectives.
backend/EXPENSE_SERVICE_COMPLETION_SUMMARY.md (1)
1-135: Excellent comprehensive documentation.This completion summary provides thorough coverage of the expense service implementation with clear structure, proper formatting, and complete feature documentation. It effectively communicates the project status and implementation details.
backend/test_expense_service.py
Outdated
| def test_expense_apis(): | ||
| """Test expense API endpoints""" | ||
|
|
||
| print("🧪 Testing Expense Service APIs...") | ||
|
|
||
| # Test health check first | ||
| try: | ||
| response = requests.get(f"{BASE_URL}/health") | ||
| if response.status_code == 200: | ||
| print("✅ Server is healthy") | ||
| else: | ||
| print("❌ Server health check failed") | ||
| return | ||
| except requests.exceptions.ConnectionError: | ||
| print("❌ Cannot connect to server. Make sure it's running on localhost:8000") | ||
| return | ||
|
|
||
| # Note: These tests require authentication and valid group/user IDs | ||
| # In a real scenario, you would need to: | ||
| # 1. Create a test user and get auth token | ||
| # 2. Create a test group | ||
| # 3. Add members to the group | ||
|
|
||
| print("\n📋 API Endpoints Available:") | ||
| print(" POST /groups/{group_id}/expenses - Create expense") | ||
| print(" GET /groups/{group_id}/expenses - List expenses") | ||
| print(" GET /groups/{group_id}/expenses/{expense_id} - Get expense") | ||
| print(" PATCH /groups/{group_id}/expenses/{expense_id} - Update expense") | ||
| print(" DELETE /groups/{group_id}/expenses/{expense_id} - Delete expense") | ||
| print(" POST /groups/{group_id}/settlements - Manual settlement") | ||
| print(" GET /groups/{group_id}/settlements - List settlements") | ||
| print(" POST /groups/{group_id}/settlements/optimize - Optimize settlements") | ||
| print(" GET /users/me/friends-balance - Friend balances") | ||
| print(" GET /users/me/balance-summary - Balance summary") | ||
| print(" GET /groups/{group_id}/analytics - Group analytics") | ||
|
|
||
| print("\n💡 Settlement Algorithms:") | ||
| print(" • Normal: Simplifies direct relationships only") | ||
| print(" • Advanced: Graph optimization with minimal transactions") | ||
|
|
||
| print("\n🔧 To test with real data:") | ||
| print(" 1. Start the server: python -m uvicorn main:app --reload") | ||
| print(" 2. Visit http://localhost:8000/docs for interactive API documentation") | ||
| print(" 3. Create a user account and group through the auth endpoints") | ||
| print(" 4. Use the group ID to test expense endpoints") | ||
|
|
||
| # Test split validation logic | ||
| print("\n🧮 Testing Split Validation Logic:") | ||
|
|
||
| def validate_splits(amount, splits): | ||
| """Test split validation""" | ||
| total_split = sum(split['amount'] for split in splits) | ||
| valid = abs(total_split - amount) <= 0.01 | ||
| return valid, total_split | ||
|
|
||
| # Test cases | ||
| test_cases = [ | ||
| { | ||
| "name": "Valid equal split", | ||
| "amount": 100.0, | ||
| "splits": [ | ||
| {"userId": "user_a", "amount": 50.0}, | ||
| {"userId": "user_b", "amount": 50.0} | ||
| ] | ||
| }, | ||
| { | ||
| "name": "Valid unequal split", | ||
| "amount": 100.0, | ||
| "splits": [ | ||
| {"userId": "user_a", "amount": 60.0}, | ||
| {"userId": "user_b", "amount": 40.0} | ||
| ] | ||
| }, | ||
| { | ||
| "name": "Invalid split (doesn't sum)", | ||
| "amount": 100.0, | ||
| "splits": [ | ||
| {"userId": "user_a", "amount": 45.0}, | ||
| {"userId": "user_b", "amount": 50.0} # Total 95, but amount is 100 | ||
| ] | ||
| }, | ||
| { | ||
| "name": "Valid three-way split", | ||
| "amount": 100.0, | ||
| "splits": [ | ||
| {"userId": "user_a", "amount": 33.33}, | ||
| {"userId": "user_b", "amount": 33.33}, | ||
| {"userId": "user_c", "amount": 33.34} | ||
| ] | ||
| } | ||
| ] | ||
|
|
||
| for test_case in test_cases: | ||
| valid, total = validate_splits(test_case["amount"], test_case["splits"]) | ||
| status = "✅" if valid else "❌" | ||
| print(f" {status} {test_case['name']}: ${test_case['amount']} -> ${total}") | ||
|
|
||
| # Test settlement algorithm logic | ||
| print("\n⚖️ Testing Settlement Algorithm Logic:") | ||
|
|
||
| def calculate_normal_settlements(settlements): | ||
| """Simulate normal settlement algorithm""" | ||
| net_balances = {} | ||
|
|
||
| for settlement in settlements: | ||
| payer = settlement['payerId'] | ||
| payee = settlement['payeeId'] | ||
| amount = settlement['amount'] | ||
|
|
||
| if payer not in net_balances: | ||
| net_balances[payer] = {} | ||
| if payee not in net_balances: | ||
| net_balances[payee] = {} | ||
| if payee not in net_balances[payer]: | ||
| net_balances[payer][payee] = 0 | ||
| if payer not in net_balances[payee]: | ||
| net_balances[payee][payer] = 0 | ||
|
|
||
| net_balances[payer][payee] += amount | ||
|
|
||
| optimized = [] | ||
| for payer in net_balances: | ||
| for payee in net_balances[payer]: | ||
| if payee in net_balances and payer in net_balances[payee]: | ||
| net_amount = net_balances[payer][payee] - net_balances[payee][payer] | ||
| if net_amount > 0.01: | ||
| optimized.append({ | ||
| 'from': payer, | ||
| 'to': payee, | ||
| 'amount': net_amount | ||
| }) | ||
|
|
||
| return optimized | ||
|
|
||
| def calculate_advanced_settlements(settlements): | ||
| """Simulate advanced settlement algorithm""" | ||
| user_balances = {} | ||
|
|
||
| for settlement in settlements: | ||
| payer = settlement['payerId'] | ||
| payee = settlement['payeeId'] | ||
| amount = settlement['amount'] | ||
|
|
||
| if payee not in user_balances: | ||
| user_balances[payee] = 0 | ||
| if payer not in user_balances: | ||
| user_balances[payer] = 0 | ||
|
|
||
| user_balances[payee] += amount # Payee owes money | ||
| user_balances[payer] -= amount # Payer is owed money | ||
|
|
||
| debtors = [[uid, bal] for uid, bal in user_balances.items() if bal > 0.01] | ||
| creditors = [[uid, -bal] for uid, bal in user_balances.items() if bal < -0.01] | ||
|
|
||
| debtors.sort(key=lambda x: x[1], reverse=True) | ||
| creditors.sort(key=lambda x: x[1], reverse=True) | ||
|
|
||
| optimized = [] | ||
| i, j = 0, 0 | ||
|
|
||
| while i < len(debtors) and j < len(creditors): | ||
| debtor_id, debt_amount = debtors[i] | ||
| creditor_id, credit_amount = creditors[j] | ||
|
|
||
| settlement_amount = min(debt_amount, credit_amount) | ||
|
|
||
| if settlement_amount > 0.01: | ||
| optimized.append({ | ||
| 'from': debtor_id, | ||
| 'to': creditor_id, | ||
| 'amount': settlement_amount | ||
| }) | ||
|
|
||
| debtors[i][1] -= settlement_amount | ||
| creditors[j][1] -= settlement_amount | ||
|
|
||
| if debtors[i][1] <= 0.01: | ||
| i += 1 | ||
| if creditors[j][1] <= 0.01: | ||
| j += 1 | ||
|
|
||
| return optimized | ||
|
|
||
| # Test scenario: Better example for advanced algorithm | ||
| # Alice paid $100 for Bob (Bob owes Alice $100) | ||
| # Bob paid $100 for Charlie (Charlie owes Bob $100) | ||
| # Expected optimized: Charlie pays Alice $100 directly | ||
| test_settlements = [ | ||
| {'payerId': 'Alice', 'payeeId': 'Bob', 'amount': 100}, # Bob owes Alice $100 | ||
| {'payerId': 'Bob', 'payeeId': 'Charlie', 'amount': 100} # Charlie owes Bob $100 | ||
| ] | ||
|
|
||
| print(f" Test scenario:") | ||
| print(f" Alice paid for Bob: Bob owes Alice $100") | ||
| print(f" Bob paid for Charlie: Charlie owes Bob $100") | ||
| print(f" Expected optimization: Charlie pays Alice $100 directly") | ||
|
|
||
| normal_result = calculate_normal_settlements(test_settlements) | ||
| advanced_result = calculate_advanced_settlements(test_settlements) | ||
|
|
||
| print(f" Original transactions: {len(test_settlements)}") | ||
| print(f" Normal algorithm: {len(normal_result)} transactions") | ||
| for settlement in normal_result: | ||
| print(f" {settlement['from']} pays {settlement['to']} ${settlement['amount']:.2f}") | ||
|
|
||
| print(f" Advanced algorithm: {len(advanced_result)} transactions") | ||
| for settlement in advanced_result: | ||
| print(f" {settlement['from']} pays {settlement['to']} ${settlement['amount']:.2f}") | ||
|
|
||
| # Debug the algorithm | ||
| print(f"\n🔍 Advanced Algorithm Debug:") | ||
| user_balances = {} | ||
| for settlement in test_settlements: | ||
| payer = settlement['payerId'] | ||
| payee = settlement['payeeId'] | ||
| amount = settlement['amount'] | ||
|
|
||
| if payee not in user_balances: | ||
| user_balances[payee] = 0 | ||
| if payer not in user_balances: | ||
| user_balances[payer] = 0 | ||
|
|
||
| user_balances[payee] += amount # Payee owes money | ||
| user_balances[payer] -= amount # Payer is owed money | ||
|
|
||
| print(f" User balances: {user_balances}") | ||
| debtors = [[uid, bal] for uid, bal in user_balances.items() if bal > 0.01] | ||
| creditors = [[uid, -bal] for uid, bal in user_balances.items() if bal < -0.01] | ||
| print(f" Debtors: {debtors}") | ||
| print(f" Creditors: {creditors}") | ||
|
|
||
| # Manually run the two-pointer algorithm with debug | ||
| optimized_debug = [] | ||
| i, j = 0, 0 | ||
|
|
||
| while i < len(debtors) and j < len(creditors): | ||
| debtor_id, debt_amount = debtors[i] | ||
| creditor_id, credit_amount = creditors[j] | ||
|
|
||
| print(f" Processing: {debtor_id} owes ${debt_amount}, {creditor_id} owed ${credit_amount}") | ||
|
|
||
| settlement_amount = min(debt_amount, credit_amount) | ||
|
|
||
| if settlement_amount > 0.01: | ||
| optimized_debug.append({ | ||
| 'from': debtor_id, | ||
| 'to': creditor_id, | ||
| 'amount': settlement_amount | ||
| }) | ||
| print(f" Adding settlement: {debtor_id} -> {creditor_id} ${settlement_amount}") | ||
|
|
||
| debtors[i][1] -= settlement_amount | ||
| creditors[j][1] -= settlement_amount | ||
|
|
||
| print(f" After settlement: {debtor_id} remaining: ${debtors[i][1]}, {creditor_id} remaining: ${creditors[j][1]}") | ||
|
|
||
| if debtors[i][1] <= 0.01: | ||
| i += 1 | ||
| if creditors[j][1] <= 0.01: | ||
| j += 1 | ||
|
|
||
| print(f" Manual debug result: {optimized_debug}") | ||
|
|
||
| print("\n🔧 Testing PATCH Endpoint Specifically:") | ||
| print(" 1. First, create an expense using POST /groups/{group_id}/expenses") | ||
| print(" 2. Note the returned expense ID") | ||
| print(" 3. Use the debug endpoint: GET /groups/{group_id}/expenses/{expense_id}/debug") | ||
| print(" 4. Test PATCH with simple update: PATCH /groups/{group_id}/expenses/{expense_id}") | ||
| print(" Body: {\"description\": \"Updated description\"}") | ||
| print(" 5. Check server logs for detailed error messages") | ||
|
|
||
| print("\n🔍 Sample PATCH requests to test:") | ||
| print(" • Update description only:") | ||
| print(" PATCH /groups/{group_id}/expenses/{expense_id}") | ||
| print(" {\"description\": \"New description\"}") | ||
|
|
||
| print(" • Update amount only:") | ||
| print(" PATCH /groups/{group_id}/expenses/{expense_id}") | ||
| print(" {\"amount\": 150.50}") | ||
|
|
||
| print(" • Update amount and splits:") | ||
| print(" PATCH /groups/{group_id}/expenses/{expense_id}") | ||
| print(" {") | ||
| print(" \"amount\": 150.0,") | ||
| print(" \"splits\": [") | ||
| print(" {\"userId\": \"user_a\", \"amount\": 75.0},") | ||
| print(" {\"userId\": \"user_b\", \"amount\": 75.0}") | ||
| print(" ]") | ||
| print(" }") | ||
|
|
||
| print("\n⚠️ Common 500 Error Causes:") | ||
| print(" • Invalid ObjectId format for expense_id") | ||
| print(" • User doesn't have permission to edit expense") | ||
| print(" • MongoDB connection issues") | ||
| print(" • Validation errors in splits/amount") | ||
| print(" • Missing required fields in database") | ||
|
|
||
| print("\n🎉 Expense Service API is ready!") | ||
| print(" Visit http://localhost:8000/docs for complete API documentation") | ||
|
|
||
| if __name__ == "__main__": | ||
| test_expense_apis() |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Refactor the monolithic function into smaller, focused functions.
The test_expense_apis() function has too many responsibilities and exceeds complexity limits (27 local variables, 159 statements). Consider breaking it down into separate functions like test_health_check(), test_split_validation(), test_settlement_algorithms(), etc.
This would improve:
- Code maintainability and readability
- Individual testability of components
- Compliance with single responsibility principle
+def test_health_check():
+ """Test server health check"""
+ # Move lines 19-28 here
+
+def test_split_validation():
+ """Test split validation logic"""
+ # Move lines 59-108 here
+
+def test_settlement_algorithms():
+ """Test settlement algorithm logic"""
+ # Move lines 110-274 here
def test_expense_apis():
- """Test expense API endpoints"""
+ """Orchestrate all expense API tests"""
+ test_health_check()
+ test_split_validation()
+ test_settlement_algorithms()
# Keep API documentation and instructionsCommittable suggestion skipped: line range outside the PR's diff.
🧰 Tools
🪛 Ruff (0.11.9)
205-205: f-string without any placeholders
Remove extraneous f prefix
(F541)
206-206: f-string without any placeholders
Remove extraneous f prefix
(F541)
207-207: f-string without any placeholders
Remove extraneous f prefix
(F541)
208-208: f-string without any placeholders
Remove extraneous f prefix
(F541)
223-223: f-string without any placeholders
Remove extraneous f prefix
(F541)
🪛 Flake8 (7.2.0)
[error] 13-13: expected 2 blank lines, found 1
(E302)
[error] 205-205: f-string is missing placeholders
(F541)
[error] 206-206: f-string is missing placeholders
(F541)
[error] 207-207: f-string is missing placeholders
(F541)
[error] 208-208: f-string is missing placeholders
(F541)
[error] 223-223: f-string is missing placeholders
(F541)
[error] 313-313: expected 2 blank lines after class or function definition, found 1
(E305)
🪛 Pylint (3.3.7)
[refactor] 13-13: Too many local variables (27/15)
(R0914)
[refactor] 147-147: Too many local variables (16/15)
(R0914)
[refactor] 13-13: Too many branches (13/12)
(R0912)
[refactor] 13-13: Too many statements (159/50)
(R0915)
🤖 Prompt for AI Agents
In backend/test_expense_service.py around lines 13 to 314, the
test_expense_apis() function is too large and handles multiple unrelated tasks,
making it hard to maintain and test. Refactor by extracting distinct parts into
smaller functions such as test_health_check(), test_split_validation(),
test_settlement_algorithms(), and test_patch_endpoint_tests(). Move the relevant
code blocks into these functions and call them sequentially from a simplified
main test_expense_apis() to improve readability, maintainability, and adherence
to the single responsibility principle.
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (5)
backend/tests/expenses/test_expense_service.py (1)
5-5: Remove unused import.-import asynciobackend/app/expenses/service.py (1)
1-1: Remove unused imports.-from typing import List, Dict, Any, Optional, Tuple +from typing import List, Dict, Any, Optional from datetime import datetime, timedelta from bson import ObjectId from app.database import mongodb from app.expenses.schemas import ( - ExpenseCreateRequest, ExpenseUpdateRequest, ExpenseResponse, Settlement, - OptimizedSettlement, SettlementCreateRequest, SettlementStatus, SplitType + ExpenseCreateRequest, ExpenseUpdateRequest, ExpenseResponse, Settlement, + OptimizedSettlement, SettlementCreateRequest, SettlementStatus ) -import asyncio -from collections import defaultdict, deque +from collections import defaultdictAlso applies to: 7-7, 9-10
backend/app/expenses/schemas.py (3)
31-45: Migratevalidate_splits_sumto Pydantic v2 syntax.Same feedback given on the previous commit still applies—the validator must be rewritten using
@field_validator, be aclassmethod, and receive aFieldValidationInfoobject.
See earlier review for concrete diff.
56-63: Ditto forExpenseUpdateRequest.validate_splits_sum.Validator still uses v1 API; please adapt to
@field_validatoras already requested.
1-1: Replace deprecated@validatorimport with@field_validator.Pydantic v2 deprecates the
validatordecorator; keeping the old import breaks forward-compatibility and triggers runtime warnings flagged in CI.
Add the v2 decorator and drop the v1 one:-from pydantic import BaseModel, Field, validator +from pydantic import BaseModel, Field, field_validator
🧹 Nitpick comments (3)
backend/app/groups/schemas.py (1)
57-68: Consider addingmodel_configto remaining models for consistency.While
LeaveGroupResponse,DeleteGroupResponse, andRemoveMemberResponsedon't use field aliases and thus don't strictly needpopulate_by_name, consider adding the configuration to all models for consistency across the codebase.class LeaveGroupResponse(BaseModel): success: bool message: str + + model_config = {"populate_by_name": True} class DeleteGroupResponse(BaseModel): success: bool message: str + + model_config = {"populate_by_name": True} class RemoveMemberResponse(BaseModel): success: bool message: str + + model_config = {"populate_by_name": True}backend/app/expenses/service.py (1)
14-16: Remove unnecessary empty__init__method.The
__init__method that only containspassis not needed in Python.class ExpenseService: - def __init__(self): - pass - @property def expenses_collection(self):backend/app/expenses/schemas.py (1)
6-23: Style: one-blank-line rule violations flagged by Flake8.Numerous class declarations are missing the required blank line before them (
E302). Runningruff --fixorflake8auto-formatting targets will clean this up.Also applies to: 49-66, 67-240
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
backend/app/auth/schemas.py(2 hunks)backend/app/expenses/schemas.py(1 hunks)backend/app/expenses/service.py(1 hunks)backend/app/groups/schemas.py(2 hunks)backend/app/user/schemas.py(1 hunks)backend/tests/expenses/test_expense_service.py(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
backend/app/auth/schemas.py (1)
backend/app/auth/routes.py (1)
refresh_token(157-198)
🪛 Flake8 (7.2.0)
backend/app/expenses/service.py
[error] 1-1: 'typing.Tuple' imported but unused
(F401)
[error] 5-5: 'app.expenses.schemas.SplitType' imported but unused
(F401)
[error] 9-9: 'asyncio' imported but unused
(F401)
[error] 10-10: 'collections.deque' imported but unused
(F401)
[error] 13-13: expected 2 blank lines, found 1
(E302)
[error] 128-128: continuation line under-indented for visual indent
(E128)
[error] 129-129: continuation line under-indented for visual indent
(E128)
[error] 243-243: local variable 'e' is assigned to but never used
(F841)
[error] 295-295: do not use bare 'except'
(E722)
[error] 582-582: continuation line under-indented for visual indent
(E128)
[error] 645-645: continuation line under-indented for visual indent
(E128)
[error] 988-988: continuation line under-indented for visual indent
(E128)
[error] 1095-1095: expected 2 blank lines after class or function definition, found 0
(E305)
backend/app/auth/schemas.py
[error] 13-13: expected 2 blank lines, found 1
(E302)
[error] 19-19: expected 2 blank lines, found 1
(E302)
[error] 24-24: expected 2 blank lines, found 1
(E302)
[error] 29-29: expected 2 blank lines, found 1
(E302)
[error] 34-34: expected 2 blank lines, found 1
(E302)
[error] 40-40: expected 2 blank lines, found 1
(E302)
[error] 56-56: expected 2 blank lines, found 1
(E302)
[error] 63-63: expected 2 blank lines, found 1
(E302)
backend/app/expenses/schemas.py
[error] 6-6: expected 2 blank lines, found 1
(E302)
[error] 11-11: expected 2 blank lines, found 1
(E302)
[error] 16-16: expected 2 blank lines, found 1
(E302)
[error] 23-23: expected 2 blank lines, found 1
(E302)
[error] 49-49: expected 2 blank lines, found 1
(E302)
[error] 67-67: expected 2 blank lines, found 1
(E302)
[error] 76-76: expected 2 blank lines, found 1
(E302)
[error] 85-85: expected 2 blank lines, found 1
(E302)
[error] 102-102: expected 2 blank lines, found 1
(E302)
[error] 118-118: expected 2 blank lines, found 1
(E302)
[error] 128-128: expected 2 blank lines, found 1
(E302)
[error] 135-135: expected 2 blank lines, found 1
(E302)
[error] 142-142: expected 2 blank lines, found 1
(E302)
[error] 149-149: expected 2 blank lines, found 1
(E302)
[error] 158-158: expected 2 blank lines, found 1
(E302)
[error] 164-164: expected 2 blank lines, found 1
(E302)
[error] 172-172: expected 2 blank lines, found 1
(E302)
[error] 184-184: expected 2 blank lines, found 1
(E302)
[error] 192-192: expected 2 blank lines, found 1
(E302)
[error] 203-203: expected 2 blank lines, found 1
(E302)
[error] 209-209: expected 2 blank lines, found 1
(E302)
[error] 218-218: expected 2 blank lines, found 1
(E302)
[error] 229-229: expected 2 blank lines, found 1
(E302)
[error] 235-235: expected 2 blank lines, found 1
(E302)
backend/app/groups/schemas.py
[error] 12-12: expected 2 blank lines, found 1
(E302)
[error] 19-19: expected 2 blank lines, found 1
(E302)
[error] 42-42: expected 2 blank lines, found 1
(E302)
[error] 47-47: expected 2 blank lines, found 1
(E302)
[error] 52-52: expected 2 blank lines, found 1
(E302)
backend/app/user/schemas.py
[error] 23-23: expected 2 blank lines, found 1
(E302)
backend/tests/expenses/test_expense_service.py
[error] 5-5: 'asyncio' imported but unused
(F401)
[error] 7-7: expected 2 blank lines, found 1
(E302)
[error] 24-24: expected 2 blank lines, found 1
(E302)
[error] 41-41: expected 2 blank lines, found 1
(E302)
[error] 65-65: expected 2 blank lines, found 1
(E302)
[error] 91-91: expected 2 blank lines after class or function definition, found 1
(E305)
🪛 Ruff (0.11.9)
backend/app/expenses/service.py
1-1: typing.Tuple imported but unused
Remove unused import: typing.Tuple
(F401)
7-7: app.expenses.schemas.SplitType imported but unused
Remove unused import: app.expenses.schemas.SplitType
(F401)
9-9: asyncio imported but unused
Remove unused import: asyncio
(F401)
10-10: collections.deque imported but unused
Remove unused import: collections.deque
(F401)
243-243: Local variable e is assigned to but never used
Remove assignment to unused variable e
(F841)
244-244: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
295-295: Do not use bare except
(E722)
356-356: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
backend/tests/expenses/test_expense_service.py
5-5: asyncio imported but unused
Remove unused import: asyncio
(F401)
🪛 Pylint (3.3.7)
backend/app/expenses/service.py
[refactor] 127-127: Too many arguments (8/5)
(R0913)
[refactor] 127-127: Too many positional arguments (8/5)
(R0917)
[refactor] 127-127: Too many local variables (21/15)
(R0914)
[refactor] 236-236: Too many local variables (18/15)
(R0914)
[refactor] 236-236: Too many branches (22/12)
(R0912)
[refactor] 236-236: Too many statements (63/50)
(R0915)
[refactor] 380-383: Unnecessary "else" after "return", remove the "else" and de-indent the code inside it
(R1705)
[refactor] 437-437: Too many local variables (21/15)
(R0914)
[refactor] 581-581: Too many arguments (6/5)
(R0913)
[refactor] 581-581: Too many positional arguments (6/5)
(R0917)
[refactor] 644-644: Too many arguments (6/5)
(R0913)
[refactor] 644-644: Too many positional arguments (6/5)
(R0917)
[refactor] 690-690: Too many local variables (22/15)
(R0914)
[refactor] 795-795: Too many local variables (19/15)
(R0914)
[refactor] 987-987: Too many arguments (6/5)
(R0913)
[refactor] 987-987: Too many positional arguments (6/5)
(R0917)
[refactor] 987-987: Too many local variables (31/15)
(R0914)
[refactor] 987-987: Too many branches (16/12)
(R0912)
backend/app/auth/schemas.py
[refactor] 13-13: Too few public methods (0/2)
(R0903)
[refactor] 19-19: Too few public methods (0/2)
(R0903)
[refactor] 24-24: Too few public methods (0/2)
(R0903)
[refactor] 29-29: Too few public methods (0/2)
(R0903)
[refactor] 34-34: Too few public methods (0/2)
(R0903)
[refactor] 40-40: Too few public methods (0/2)
(R0903)
[refactor] 56-56: Too few public methods (0/2)
(R0903)
[refactor] 63-63: Too few public methods (0/2)
(R0903)
backend/app/expenses/schemas.py
[refactor] 16-16: Too few public methods (0/2)
(R0903)
[error] 32-32: Method 'validate_splits_sum' should have "self" as first argument
(E0213)
[refactor] 23-23: Too few public methods (1/2)
(R0903)
[error] 57-57: Method 'validate_splits_sum' should have "self" as first argument
(E0213)
[refactor] 49-49: Too few public methods (1/2)
(R0903)
[refactor] 67-67: Too few public methods (0/2)
(R0903)
[refactor] 76-76: Too few public methods (0/2)
(R0903)
[refactor] 85-85: Too few public methods (0/2)
(R0903)
[refactor] 102-102: Too few public methods (0/2)
(R0903)
[refactor] 118-118: Too few public methods (0/2)
(R0903)
[refactor] 128-128: Too few public methods (0/2)
(R0903)
[refactor] 135-135: Too few public methods (0/2)
(R0903)
[refactor] 142-142: Too few public methods (0/2)
(R0903)
[refactor] 149-149: Too few public methods (0/2)
(R0903)
[refactor] 158-158: Too few public methods (0/2)
(R0903)
[refactor] 164-164: Too few public methods (0/2)
(R0903)
[refactor] 172-172: Too few public methods (0/2)
(R0903)
[refactor] 184-184: Too few public methods (0/2)
(R0903)
[refactor] 192-192: Too few public methods (0/2)
(R0903)
[refactor] 203-203: Too few public methods (0/2)
(R0903)
[refactor] 209-209: Too few public methods (0/2)
(R0903)
[refactor] 218-218: Too few public methods (0/2)
(R0903)
[refactor] 229-229: Too few public methods (0/2)
(R0903)
[refactor] 235-235: Too few public methods (0/2)
(R0903)
backend/app/groups/schemas.py
[refactor] 12-12: Too few public methods (0/2)
(R0903)
[refactor] 19-19: Too few public methods (0/2)
(R0903)
[refactor] 42-42: Too few public methods (0/2)
(R0903)
[refactor] 47-47: Too few public methods (0/2)
(R0903)
[refactor] 52-52: Too few public methods (0/2)
(R0903)
backend/app/user/schemas.py
[refactor] 23-23: Too few public methods (0/2)
(R0903)
🪛 GitHub Actions: Run Tests
backend/app/expenses/schemas.py
[warning] 31-31: PydanticDeprecatedSince20: Pydantic V1 style '@validator' validators are deprecated. Migrate to Pydantic V2 style '@field_validator'.
[warning] 56-56: PydanticDeprecatedSince20: Pydantic V1 style '@validator' validators are deprecated. Migrate to Pydantic V2 style '@field_validator'.
backend/tests/expenses/test_expense_service.py
[error] 18-18: AttributeError: 'NoneType' object has no attribute 'settlements' in test_settlement_algorithm_normal. The test failed due to accessing 'mongodb.database.settlements' when 'mongodb.database' is None.
[error] 33-33: AttributeError: 'NoneType' object has no attribute 'settlements' in test_settlement_algorithm_advanced. The test failed due to accessing 'mongodb.database.settlements' when 'mongodb.database' is None.
🔇 Additional comments (2)
backend/app/user/schemas.py (1)
21-21: LGTM!The addition of
model_config = {"populate_by_name": True}is consistent with the schema updates across other modules, ensuring uniform Pydantic model behavior throughout the application.Also applies to: 27-27
backend/app/auth/schemas.py (1)
11-11: Good modernization of Pydantic configuration!The systematic update from the old
Configclass pattern to the newmodel_configattribute is the recommended approach for newer Pydantic versions. This ensures consistent field population behavior across all authentication models.Also applies to: 17-17, 22-22, 27-27, 32-32, 38-38, 43-43, 54-54, 61-61, 67-67
| @pytest.mark.asyncio | ||
| async def test_settlement_algorithm_normal(): | ||
| """Test normal settlement algorithm with mocked DB""" | ||
| service = ExpenseService() | ||
| group_id = "test_group_123" | ||
| # Mock settlements in DB | ||
| settlements = [ | ||
| {"payerId": "user_a", "payeeId": "user_b", "amount": 100, "payerName": "Alice", "payeeName": "Bob"}, | ||
| {"payerId": "user_b", "payeeId": "user_a", "amount": 50, "payerName": "Bob", "payeeName": "Alice"}, | ||
| {"payerId": "user_a", "payeeId": "user_c", "amount": 75, "payerName": "Alice", "payeeName": "Charlie"}, | ||
| ] | ||
| with patch.object(service.settlements_collection, 'find', return_value=AsyncMock(to_list=AsyncMock(return_value=settlements))): | ||
| optimized = await service._calculate_normal_settlements(group_id) | ||
| # Alice owes Bob 50, Alice is owed 75 by Charlie | ||
| assert any(o.fromUserId == "user_a" and o.toUserId == "user_b" and abs(o.amount - 50) < 0.01 for o in optimized) | ||
| assert any(o.fromUserId == "user_c" and o.toUserId == "user_a" and abs(o.amount - 75) < 0.01 for o in optimized) | ||
|
|
There was a problem hiding this comment.
Fix database initialization for tests.
The tests are failing because ExpenseService properties try to access mongodb.database which is None in the test environment. The current patching approach fails because the property is evaluated before the patch is applied.
Consider one of these approaches:
+from unittest.mock import MagicMock
+
@pytest.mark.asyncio
async def test_settlement_algorithm_normal():
"""Test normal settlement algorithm with mocked DB"""
- service = ExpenseService()
+ # Mock the database before service instantiation
+ from app.database import mongodb
+ mongodb.database = MagicMock()
+ mongodb.database.settlements.find.return_value = AsyncMock(to_list=AsyncMock(return_value=settlements))
+
+ service = ExpenseService()
group_id = "test_group_123"
# Mock settlements in DB
settlements = [
{"payerId": "user_a", "payeeId": "user_b", "amount": 100, "payerName": "Alice", "payeeName": "Bob"},
{"payerId": "user_b", "payeeId": "user_a", "amount": 50, "payerName": "Bob", "payeeName": "Alice"},
{"payerId": "user_a", "payeeId": "user_c", "amount": 75, "payerName": "Alice", "payeeName": "Charlie"},
]
- with patch.object(service.settlements_collection, 'find', return_value=AsyncMock(to_list=AsyncMock(return_value=settlements))):
- optimized = await service._calculate_normal_settlements(group_id)
+ optimized = await service._calculate_normal_settlements(group_id)Or better yet, use dependency injection to make the service more testable.
Committable suggestion skipped: line range outside the PR's diff.
🧰 Tools
🪛 Flake8 (7.2.0)
[error] 7-7: expected 2 blank lines, found 1
(E302)
🪛 GitHub Actions: Run Tests
[error] 18-18: AttributeError: 'NoneType' object has no attribute 'settlements' in test_settlement_algorithm_normal. The test failed due to accessing 'mongodb.database.settlements' when 'mongodb.database' is None.
🤖 Prompt for AI Agents
In backend/tests/expenses/test_expense_service.py around lines 7 to 23, the test
fails because ExpenseService tries to access mongodb.database which is None in
the test environment, and patching after instantiation is too late. To fix this,
refactor ExpenseService to accept the database or collection dependencies via
constructor parameters (dependency injection) so you can pass mocked collections
directly in tests. Then update the test to instantiate ExpenseService with these
mocked dependencies instead of patching properties after creation.
| try: | ||
| user = await self.users_collection.find_one({"_id": ObjectId(user_id)}) | ||
| user_name = user.get("name", "Unknown User") if user else "Unknown User" | ||
| except: | ||
| user_name = "Unknown User" |
There was a problem hiding this comment.
Avoid bare except clause.
Bare except: catches all exceptions including system exceptions like KeyboardInterrupt.
try:
user = await self.users_collection.find_one({"_id": ObjectId(user_id)})
user_name = user.get("name", "Unknown User") if user else "Unknown User"
- except:
+ except Exception:
user_name = "Unknown User"📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| try: | |
| user = await self.users_collection.find_one({"_id": ObjectId(user_id)}) | |
| user_name = user.get("name", "Unknown User") if user else "Unknown User" | |
| except: | |
| user_name = "Unknown User" | |
| try: | |
| user = await self.users_collection.find_one({"_id": ObjectId(user_id)}) | |
| user_name = user.get("name", "Unknown User") if user else "Unknown User" | |
| except Exception: | |
| user_name = "Unknown User" |
🧰 Tools
🪛 Flake8 (7.2.0)
[error] 295-295: do not use bare 'except'
(E722)
🪛 Ruff (0.11.9)
295-295: Do not use bare except
(E722)
🤖 Prompt for AI Agents
In backend/app/expenses/service.py around lines 292 to 296, the bare except
clause should be replaced with a more specific exception to avoid catching
system exceptions like KeyboardInterrupt. Identify the likely exceptions that
can be raised by the code inside the try block, such as those from ObjectId
conversion or database access, and catch only those specific exceptions instead
of using a bare except. This will make the error handling safer and more
precise.
| try: | ||
| expense_obj_id = ObjectId(expense_id) | ||
| except Exception as e: | ||
| raise ValueError(f"Invalid expense ID format: {expense_id}") |
There was a problem hiding this comment.
Use the exception variable in the error message.
The exception is caught but not used in the error message.
try:
expense_obj_id = ObjectId(expense_id)
except Exception as e:
- raise ValueError(f"Invalid expense ID format: {expense_id}")
+ raise ValueError(f"Invalid expense ID format: {expense_id}") from e📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| try: | |
| expense_obj_id = ObjectId(expense_id) | |
| except Exception as e: | |
| raise ValueError(f"Invalid expense ID format: {expense_id}") | |
| try: | |
| expense_obj_id = ObjectId(expense_id) | |
| except Exception as e: | |
| raise ValueError(f"Invalid expense ID format: {expense_id}") from e |
🧰 Tools
🪛 Flake8 (7.2.0)
[error] 243-243: local variable 'e' is assigned to but never used
(F841)
🪛 Ruff (0.11.9)
243-243: Local variable e is assigned to but never used
Remove assignment to unused variable e
(F841)
244-244: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
🤖 Prompt for AI Agents
In backend/app/expenses/service.py around lines 241 to 244, the caught exception
variable 'e' is not included in the error message when raising the ValueError.
Modify the error message to include the exception details by appending or
formatting the string with the exception variable 'e' to provide more context
about the invalid expense ID format.
| tags: Optional[List[str]] = [] | ||
| receiptUrls: Optional[List[str]] = [] | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Avoid mutable list defaults.
Using plain [] in model fields shares the same list between all instances. Switch to default_factory=list (or Field(default_factory=list)) to prevent subtle mutation bugs:
- tags: Optional[List[str]] = []
- receiptUrls: Optional[List[str]] = []
+ tags: Optional[List[str]] = Field(default_factory=list)
+ receiptUrls: Optional[List[str]] = Field(default_factory=list)Apply the same pattern to every = [] occurrence in this file (ExpenseResponse, OptimizedSettlement.consolidatedExpenses, UserBalance.pendingSettlements, UserBalance.recentExpenses, etc.).
Also applies to: 93-95, 124-125, 179-181
🤖 Prompt for AI Agents
In backend/app/expenses/schemas.py around lines 28 to 30 and also at lines
93-95, 124-125, and 179-181, replace all mutable list default values defined as
`= []` with `default_factory=list` using Pydantic's Field function. This change
prevents all instances from sharing the same list object and avoids potential
mutation bugs. Update each field declaration accordingly to use
`Field(default_factory=list)` instead of a plain empty list.
This reverts commit 3b45a4b.
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (7)
backend/tests/expenses/test_expense_service.py (1)
10-27: Complete the placeholder test implementation.The test contains a placeholder assertion and unused variables. Consider implementing actual validation logic for the settlement algorithm.
Do you want me to help implement the actual settlement algorithm validation logic to replace the placeholder assertion?
backend/app/expenses/service.py (4)
1-10: Remove unused imports.Several imports are not used in the code and should be removed.
-from typing import List, Dict, Any, Optional, Tuple +from typing import List, Dict, Any, Optional from datetime import datetime, timedelta from bson import ObjectId from app.database import mongodb from app.expenses.schemas import ( - ExpenseCreateRequest, ExpenseUpdateRequest, ExpenseResponse, Settlement, - OptimizedSettlement, SettlementCreateRequest, SettlementStatus, SplitType + ExpenseCreateRequest, ExpenseUpdateRequest, ExpenseResponse, Settlement, + OptimizedSettlement, SettlementCreateRequest, SettlementStatus ) -import asyncio -from collections import defaultdict, deque +from collections import defaultdict
232-235: Use the exception variable in the error message.The exception is caught but not used in the error message.
try: expense_obj_id = ObjectId(expense_id) except Exception as e: - raise ValueError(f"Invalid expense ID format: {expense_id}") + raise ValueError(f"Invalid expense ID format: {expense_id}") from e
283-287: Avoid bare except clause.Bare
except:catches all exceptions including system exceptions likeKeyboardInterrupt.try: user = await self.users_collection.find_one({"_id": ObjectId(user_id)}) user_name = user.get("name", "Unknown User") if user else "Unknown User" - except: + except Exception: user_name = "Unknown User"
407-424: Use decimal arithmetic for financial calculations.Financial calculations should use decimal arithmetic to ensure accuracy and avoid floating-point precision issues.
+from decimal import Decimal, ROUND_HALF_UP + net_amount = payer_owes_payee - payee_owes_payer - if net_amount > 0.01: # Payer owes payee + net_amount_decimal = Decimal(str(net_amount)) + if net_amount_decimal > Decimal('0.01'): # Payer owes payee optimized.append(OptimizedSettlement( fromUserId=payer, toUserId=payee, fromUserName=user_names.get(payer, "Unknown"), toUserName=user_names.get(payee, "Unknown"), - amount=round(net_amount, 2) + amount=float(net_amount_decimal.quantize(Decimal('0.01'), rounding=ROUND_HALF_UP)) ))backend/app/expenses/schemas.py (2)
1-1: Migrate to Pydantic v2 field validators.The
@validatordecorator is deprecated in Pydantic v2. Migrate to@field_validator.-from pydantic import BaseModel, Field, validator +from pydantic import BaseModel, Field, field_validator class ExpenseCreateRequest(BaseModel): # ... fields ... - @validator('splits') - def validate_splits_sum(cls, v, values): - if 'amount' in values: + @field_validator('splits') + @classmethod + def validate_splits_sum(cls, v, info): + if 'amount' in info.data: total_split = sum(split.amount for split in v) - if abs(total_split - values['amount']) > 0.01: + if abs(total_split - info.data['amount']) > 0.01: raise ValueError('Split amounts must sum to total expense amount') return vApply similar changes to the validator in
ExpenseUpdateRequest.Also applies to: 29-35, 44-51
26-27: Avoid mutable list defaults.Using plain
[]in model fields shares the same list between all instances. Switch todefault_factory=listto prevent subtle mutation bugs.- tags: Optional[List[str]] = [] - receiptUrls: Optional[List[str]] = [] + tags: Optional[List[str]] = Field(default_factory=list) + receiptUrls: Optional[List[str]] = Field(default_factory=list)Apply the same pattern to every
= []occurrence in this file (ExpenseResponse,OptimizedSettlement.consolidatedExpenses,UserBalance.pendingSettlements,UserBalance.recentExpenses, etc.).Also applies to: 83-86, 114-114, 155-156
🧹 Nitpick comments (1)
backend/app/expenses/schemas.py (1)
53-55: Migrate from Config class to model_config for consistency.Use
model_configinstead of theConfigclass for consistency with other models and Pydantic v2 best practices.- class Config: - # Allow validation to work with partial updates - validate_assignment = True + model_config = {"validate_assignment": True}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
backend/app/expenses/schemas.py(1 hunks)backend/app/expenses/service.py(1 hunks)backend/tests/expenses/test_expense_service.py(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (3)
backend/tests/expenses/test_expense_service.py (1)
backend/app/expenses/schemas.py (3)
ExpenseCreateRequest(21-35)ExpenseSplit(16-19)SplitType(6-9)
backend/app/expenses/service.py (2)
backend/app/expenses/schemas.py (8)
ExpenseCreateRequest(21-35)ExpenseUpdateRequest(37-55)ExpenseResponse(75-90)Settlement(92-106)OptimizedSettlement(108-114)SettlementCreateRequest(131-136)SettlementStatus(11-14)SplitType(6-9)backend/app/expenses/routes.py (7)
create_expense(21-33)calculate_optimized_settlements(282-310)list_group_expenses(36-55)update_expense(73-89)delete_expense(92-106)get_group_settlements(180-225)delete_settlement(261-279)
backend/app/expenses/schemas.py (1)
backend/app/auth/schemas.py (1)
Config(40-41)
🪛 Ruff (0.11.9)
backend/tests/expenses/test_expense_service.py
2-2: main.app imported but unused
Remove unused import: main.app
(F401)
3-3: app.expenses.service.expense_service imported but unused
Remove unused import: app.expenses.service.expense_service
(F401)
5-5: asyncio imported but unused
Remove unused import: asyncio
(F401)
13-13: Local variable group_id is assigned to but never used
Remove assignment to unused variable group_id
(F841)
16-16: Local variable settlements is assigned to but never used
Remove assignment to unused variable settlements
(F841)
37-37: Local variable user_balances is assigned to but never used
Remove assignment to unused variable user_balances
(F841)
backend/app/expenses/service.py
1-1: typing.Tuple imported but unused
Remove unused import: typing.Tuple
(F401)
7-7: app.expenses.schemas.SplitType imported but unused
Remove unused import: app.expenses.schemas.SplitType
(F401)
9-9: asyncio imported but unused
Remove unused import: asyncio
(F401)
10-10: collections.deque imported but unused
Remove unused import: collections.deque
(F401)
234-234: Local variable e is assigned to but never used
Remove assignment to unused variable e
(F841)
235-235: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
286-286: Do not use bare except
(E722)
347-347: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
🪛 Flake8 (7.2.0)
backend/tests/expenses/test_expense_service.py
[error] 2-2: 'main.app' imported but unused
(F401)
[error] 2-2: at least two spaces before inline comment
(E261)
[error] 3-3: 'app.expenses.service.expense_service' imported but unused
(F401)
[error] 5-5: 'asyncio' imported but unused
(F401)
[error] 9-9: expected 2 blank lines, found 1
(E302)
[error] 13-13: local variable 'group_id' is assigned to but never used
(F841)
[error] 16-16: local variable 'settlements' is assigned to but never used
(F841)
[error] 28-28: expected 2 blank lines, found 1
(E302)
[error] 37-37: local variable 'user_balances' is assigned to but never used
(F841)
[error] 78-78: expected 2 blank lines, found 1
(E302)
[error] 109-109: expected 2 blank lines, found 1
(E302)
[error] 143-143: expected 2 blank lines after class or function definition, found 1
(E305)
backend/app/expenses/service.py
[error] 1-1: 'typing.Tuple' imported but unused
(F401)
[error] 5-5: 'app.expenses.schemas.SplitType' imported but unused
(F401)
[error] 9-9: 'asyncio' imported but unused
(F401)
[error] 10-10: 'collections.deque' imported but unused
(F401)
[error] 12-12: expected 2 blank lines, found 1
(E302)
[error] 119-119: continuation line under-indented for visual indent
(E128)
[error] 120-120: continuation line under-indented for visual indent
(E128)
[error] 234-234: local variable 'e' is assigned to but never used
(F841)
[error] 286-286: do not use bare 'except'
(E722)
[error] 573-573: continuation line under-indented for visual indent
(E128)
[error] 636-636: continuation line under-indented for visual indent
(E128)
[error] 979-979: continuation line under-indented for visual indent
(E128)
[error] 1086-1086: expected 2 blank lines after class or function definition, found 0
(E305)
backend/app/expenses/schemas.py
[error] 6-6: expected 2 blank lines, found 1
(E302)
[error] 11-11: expected 2 blank lines, found 1
(E302)
[error] 16-16: expected 2 blank lines, found 1
(E302)
[error] 21-21: expected 2 blank lines, found 1
(E302)
[error] 37-37: expected 2 blank lines, found 1
(E302)
[error] 57-57: expected 2 blank lines, found 1
(E302)
[error] 66-66: expected 2 blank lines, found 1
(E302)
[error] 75-75: expected 2 blank lines, found 1
(E302)
[error] 92-92: expected 2 blank lines, found 1
(E302)
[error] 108-108: expected 2 blank lines, found 1
(E302)
[error] 116-116: expected 2 blank lines, found 1
(E302)
[error] 121-121: expected 2 blank lines, found 1
(E302)
[error] 126-126: expected 2 blank lines, found 1
(E302)
[error] 131-131: expected 2 blank lines, found 1
(E302)
[error] 138-138: expected 2 blank lines, found 1
(E302)
[error] 142-142: expected 2 blank lines, found 1
(E302)
[error] 148-148: expected 2 blank lines, found 1
(E302)
[error] 158-158: expected 2 blank lines, found 1
(E302)
[error] 164-164: expected 2 blank lines, found 1
(E302)
[error] 173-173: expected 2 blank lines, found 1
(E302)
[error] 177-177: expected 2 blank lines, found 1
(E302)
[error] 184-184: expected 2 blank lines, found 1
(E302)
[error] 193-193: expected 2 blank lines, found 1
(E302)
[error] 197-197: expected 2 blank lines, found 1
(E302)
🪛 Pylint (3.3.7)
backend/app/expenses/service.py
[refactor] 118-118: Too many arguments (8/5)
(R0913)
[refactor] 118-118: Too many positional arguments (8/5)
(R0917)
[refactor] 118-118: Too many local variables (21/15)
(R0914)
[refactor] 227-227: Too many local variables (18/15)
(R0914)
[refactor] 227-227: Too many branches (22/12)
(R0912)
[refactor] 227-227: Too many statements (63/50)
(R0915)
[refactor] 371-374: Unnecessary "else" after "return", remove the "else" and de-indent the code inside it
(R1705)
[refactor] 428-428: Too many local variables (21/15)
(R0914)
[refactor] 572-572: Too many arguments (6/5)
(R0913)
[refactor] 572-572: Too many positional arguments (6/5)
(R0917)
[refactor] 635-635: Too many arguments (6/5)
(R0913)
[refactor] 635-635: Too many positional arguments (6/5)
(R0917)
[refactor] 681-681: Too many local variables (22/15)
(R0914)
[refactor] 786-786: Too many local variables (19/15)
(R0914)
[refactor] 978-978: Too many arguments (6/5)
(R0913)
[refactor] 978-978: Too many positional arguments (6/5)
(R0917)
[refactor] 978-978: Too many local variables (31/15)
(R0914)
[refactor] 978-978: Too many branches (16/12)
(R0912)
backend/app/expenses/schemas.py
[refactor] 16-16: Too few public methods (0/2)
(R0903)
[error] 30-30: Method 'validate_splits_sum' should have "self" as first argument
(E0213)
[refactor] 21-21: Too few public methods (1/2)
(R0903)
[error] 45-45: Method 'validate_splits_sum' should have "self" as first argument
(E0213)
[refactor] 53-53: Too few public methods (0/2)
(R0903)
[refactor] 37-37: Too few public methods (1/2)
(R0903)
[refactor] 57-57: Too few public methods (0/2)
(R0903)
[refactor] 66-66: Too few public methods (0/2)
(R0903)
[refactor] 75-75: Too few public methods (0/2)
(R0903)
[refactor] 92-92: Too few public methods (0/2)
(R0903)
[refactor] 108-108: Too few public methods (0/2)
(R0903)
[refactor] 116-116: Too few public methods (0/2)
(R0903)
[refactor] 121-121: Too few public methods (0/2)
(R0903)
[refactor] 126-126: Too few public methods (0/2)
(R0903)
[refactor] 131-131: Too few public methods (0/2)
(R0903)
[refactor] 138-138: Too few public methods (0/2)
(R0903)
[refactor] 142-142: Too few public methods (0/2)
(R0903)
[refactor] 148-148: Too few public methods (0/2)
(R0903)
[refactor] 158-158: Too few public methods (0/2)
(R0903)
[refactor] 164-164: Too few public methods (0/2)
(R0903)
[refactor] 173-173: Too few public methods (0/2)
(R0903)
[refactor] 177-177: Too few public methods (0/2)
(R0903)
[refactor] 184-184: Too few public methods (0/2)
(R0903)
[refactor] 193-193: Too few public methods (0/2)
(R0903)
[refactor] 197-197: Too few public methods (0/2)
(R0903)
🔇 Additional comments (3)
backend/tests/expenses/test_expense_service.py (3)
78-108: LGTM!The test properly validates both valid and invalid expense splits.
109-142: LGTM!The test correctly validates different split types (equal and unequal).
28-77: ```shell
#!/bin/bashSearch for the advanced settlements method in the ExpenseService
rg -n "_calculate_advanced_settlements" -C 3 .
Locate the ExpenseService definition to confirm its methods
rg -n "class ExpenseService" -C 3 .
</details> </blockquote></details> </details> <!-- This is an auto-generated comment by CodeRabbit for review status -->
| # Create new settlements | ||
| await self._create_settlements_for_expense(updated_expense, user_id) | ||
| except Exception as e: | ||
| print(f"Warning: Failed to recalculate settlements: {e}") |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Use proper logging instead of print statements.
Replace print statements with Python's logging module for better production-ready logging.
import logging
logger = logging.getLogger(__name__)
# Replace line 331:
logger.warning(f"Failed to recalculate settlements: {e}")
# Replace lines 344-347:
logger.error(f"Error in update_expense: {str(e)}")
logger.error(f"Traceback:", exc_info=True)Also applies to: 344-347
🤖 Prompt for AI Agents
In backend/app/expenses/service.py at lines 331 and 344-347, replace all print
statements with proper logging calls using Python's logging module. Import
logging and create a logger instance at the top of the file. Change the print on
line 331 to logger.warning with the error message, and replace the error prints
on lines 344-347 with logger.error calls, including exc_info=True for traceback
logging.
…update response models
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
backend/app/groups/routes.py (1)
94-101: Good improvement on type safety!The change from
List[Dict[str, Any]]toList[GroupMemberWithDetails]provides better type safety and API documentation.Add a blank line before the function definition to comply with PEP8:
return LeaveGroupResponse(success=True, message="Successfully left the group") + @router.get("/{group_id}/members", response_model=List[GroupMemberWithDetails])
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
backend/app/groups/routes.py(2 hunks)backend/app/groups/schemas.py(2 hunks)backend/app/groups/service.py(4 hunks)
🧰 Additional context used
🪛 Flake8 (7.2.0)
backend/app/groups/schemas.py
[error] 10-10: expected 2 blank lines, found 1
(E302)
backend/app/groups/routes.py
[error] 94-94: expected 2 blank lines, found 1
(E302)
backend/app/groups/service.py
[error] 132-132: local variable 'e' is assigned to but never used
(F841)
[error] 301-301: local variable 'e' is assigned to but never used
(F841)
🪛 Pylint (3.3.7)
backend/app/groups/schemas.py
[refactor] 10-10: Too few public methods (0/2)
(R0903)
🪛 Ruff (0.11.9)
backend/app/groups/routes.py
97-97: Do not perform function call Depends in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
backend/app/groups/service.py
132-132: Local variable e is assigned to but never used
Remove assignment to unused variable e
(F841)
301-301: Local variable e is assigned to but never used
Remove assignment to unused variable e
(F841)
| class GroupMemberWithDetails(BaseModel): | ||
| userId: str | ||
| role: str = "member" # "admin" or "member" | ||
| joinedAt: datetime | ||
| user: Optional[dict] = None # Contains user details like name, email |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Refactor to avoid code duplication and improve type safety.
The GroupMemberWithDetails class duplicates all fields from GroupMember. Additionally, the user field uses Optional[dict] which loses type safety.
Apply this refactor to use inheritance and define a proper user schema:
+class UserDetails(BaseModel):
+ name: str
+ email: str
+ avatar: Optional[str] = None
+
-class GroupMemberWithDetails(BaseModel):
- userId: str
- role: str = "member" # "admin" or "member"
- joinedAt: datetime
- user: Optional[dict] = None # Contains user details like name, email
+class GroupMemberWithDetails(GroupMember):
+ user: Optional[UserDetails] = None📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| class GroupMemberWithDetails(BaseModel): | |
| userId: str | |
| role: str = "member" # "admin" or "member" | |
| joinedAt: datetime | |
| user: Optional[dict] = None # Contains user details like name, email | |
| class UserDetails(BaseModel): | |
| name: str | |
| email: str | |
| avatar: Optional[str] = None | |
| class GroupMemberWithDetails(GroupMember): | |
| user: Optional[UserDetails] = None |
🧰 Tools
🪛 Flake8 (7.2.0)
[error] 10-10: expected 2 blank lines, found 1
(E302)
🪛 Pylint (3.3.7)
[refactor] 10-10: Too few public methods (0/2)
(R0903)
🤖 Prompt for AI Agents
In backend/app/groups/schemas.py around lines 10 to 14, refactor
GroupMemberWithDetails to inherit from GroupMember to avoid duplicating fields.
Replace the user field's type from Optional[dict] to Optional of a properly
defined User schema class to improve type safety. Define or import a User schema
that includes user details like name and email, and use it as the type for the
user field.
backend/app/groups/service.py
Outdated
| } | ||
| } | ||
| enriched_members.append(enriched_member) | ||
| except Exception as e: |
There was a problem hiding this comment.
Remove or use the unused exception variable.
The exception variable e is assigned but never used, which could hide debugging information.
Either remove the variable or log it for debugging:
- except Exception as e:
+ except Exception:Or better, log the exception:
except Exception as e:
+ # Log for debugging
+ # logger.warning(f"Failed to fetch user details for {member_user_id}: {e}")Also applies to: 301-301
🧰 Tools
🪛 Ruff (0.11.9)
132-132: Local variable e is assigned to but never used
Remove assignment to unused variable e
(F841)
🪛 Flake8 (7.2.0)
[error] 132-132: local variable 'e' is assigned to but never used
(F841)
🤖 Prompt for AI Agents
In backend/app/groups/service.py at lines 132 and 301, the exception variable
'e' is assigned but not used, which loses valuable debugging information. Modify
the except blocks to log the exception 'e' using an appropriate logging method
or remove the variable if not needed. Preferably, include the exception details
in the log to aid debugging.
backend/app/groups/service.py
Outdated
| if member_user_id: | ||
| try: | ||
| # Fetch user details from users collection | ||
| user_obj_id = ObjectId(member_user_id) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add validation for ObjectId format before conversion.
The code attempts to convert member_user_id to ObjectId without validation, which could raise exceptions for invalid formats.
Add validation before the conversion:
if member_user_id:
try:
+ # Validate ObjectId format
+ if not ObjectId.is_valid(member_user_id):
+ raise ValueError("Invalid ObjectId format")
+
# Fetch user details from users collection
user_obj_id = ObjectId(member_user_id)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if member_user_id: | |
| try: | |
| # Fetch user details from users collection | |
| user_obj_id = ObjectId(member_user_id) | |
| if member_user_id: | |
| try: | |
| # Validate ObjectId format | |
| if not ObjectId.is_valid(member_user_id): | |
| raise ValueError("Invalid ObjectId format") | |
| # Fetch user details from users collection | |
| user_obj_id = ObjectId(member_user_id) |
🤖 Prompt for AI Agents
In backend/app/groups/service.py around lines 110 to 113, the code converts
member_user_id to an ObjectId without validating its format, which can cause
exceptions if the format is invalid. Add a validation step to check if
member_user_id is a valid ObjectId string before conversion, and handle the case
where it is invalid to prevent exceptions.
…d refactor existing member enrichment logic
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (3)
backend/app/groups/service.py (1)
21-66: Great refactoring to eliminate code duplication!The extraction of member enrichment logic into a private method successfully addresses the DRY principle violation mentioned in previous reviews. However, there are a few improvements needed:
- Unused exception variable: The exception variable
eon line 50 is caught but never used- Missing ObjectId validation: Consider validating the ObjectId format before conversion
- Complex conditional logic: The user details construction could be simplified
Apply this diff to address the unused exception variable:
- except Exception as e: + except Exception: # If user lookup fails, add member with basic infoFor ObjectId validation, consider adding this check:
try: + # Validate ObjectId format first + if not ObjectId.is_valid(member_user_id): + raise ValueError("Invalid ObjectId format") # Fetch user details from users collection user_obj_id = ObjectId(member_user_id)backend/app/expenses/schemas.py (2)
1-1: Deprecated Pydantic v1 import detected.The
validatorimport is deprecated in Pydantic v2 and should be replaced withfield_validator.
29-35: Migrate to Pydantic v2 field validators.The
@validatordecorator usage is already flagged in previous reviews and needs to be migrated to@field_validator.Also applies to: 44-51
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
backend/app/expenses/schemas.py(1 hunks)backend/app/groups/service.py(5 hunks)
🧰 Additional context used
🪛 Flake8 (7.2.0)
backend/app/expenses/schemas.py
[error] 6-6: expected 2 blank lines, found 1
(E302)
[error] 11-11: expected 2 blank lines, found 1
(E302)
[error] 16-16: expected 2 blank lines, found 1
(E302)
[error] 21-21: expected 2 blank lines, found 1
(E302)
[error] 37-37: expected 2 blank lines, found 1
(E302)
[error] 57-57: expected 2 blank lines, found 1
(E302)
[error] 70-70: expected 2 blank lines, found 1
(E302)
[error] 79-79: expected 2 blank lines, found 1
(E302)
[error] 96-96: expected 2 blank lines, found 1
(E302)
[error] 112-112: expected 2 blank lines, found 1
(E302)
[error] 120-120: expected 2 blank lines, found 1
(E302)
[error] 125-125: expected 2 blank lines, found 1
(E302)
[error] 130-130: expected 2 blank lines, found 1
(E302)
[error] 135-135: expected 2 blank lines, found 1
(E302)
[error] 142-142: expected 2 blank lines, found 1
(E302)
[error] 146-146: expected 2 blank lines, found 1
(E302)
[error] 152-152: expected 2 blank lines, found 1
(E302)
[error] 162-162: expected 2 blank lines, found 1
(E302)
[error] 168-168: expected 2 blank lines, found 1
(E302)
[error] 177-177: expected 2 blank lines, found 1
(E302)
[error] 181-181: expected 2 blank lines, found 1
(E302)
[error] 188-188: expected 2 blank lines, found 1
(E302)
[error] 197-197: expected 2 blank lines, found 1
(E302)
[error] 201-201: expected 2 blank lines, found 1
(E302)
backend/app/groups/service.py
[error] 50-50: local variable 'e' is assigned to but never used
(F841)
🪛 Pylint (3.3.7)
backend/app/expenses/schemas.py
[refactor] 16-16: Too few public methods (0/2)
(R0903)
[error] 30-30: Method 'validate_splits_sum' should have "self" as first argument
(E0213)
[refactor] 21-21: Too few public methods (1/2)
(R0903)
[error] 45-45: Method 'validate_splits_sum' should have "self" as first argument
(E0213)
[refactor] 53-53: Too few public methods (0/2)
(R0903)
[refactor] 37-37: Too few public methods (1/2)
(R0903)
[refactor] 57-57: Too few public methods (0/2)
(R0903)
[refactor] 70-70: Too few public methods (0/2)
(R0903)
[refactor] 79-79: Too few public methods (0/2)
(R0903)
[refactor] 96-96: Too few public methods (0/2)
(R0903)
[refactor] 112-112: Too few public methods (0/2)
(R0903)
[refactor] 120-120: Too few public methods (0/2)
(R0903)
[refactor] 125-125: Too few public methods (0/2)
(R0903)
[refactor] 130-130: Too few public methods (0/2)
(R0903)
[refactor] 135-135: Too few public methods (0/2)
(R0903)
[refactor] 142-142: Too few public methods (0/2)
(R0903)
[refactor] 146-146: Too few public methods (0/2)
(R0903)
[refactor] 152-152: Too few public methods (0/2)
(R0903)
[refactor] 162-162: Too few public methods (0/2)
(R0903)
[refactor] 168-168: Too few public methods (0/2)
(R0903)
[refactor] 177-177: Too few public methods (0/2)
(R0903)
[refactor] 181-181: Too few public methods (0/2)
(R0903)
[refactor] 188-188: Too few public methods (0/2)
(R0903)
[refactor] 197-197: Too few public methods (0/2)
(R0903)
[refactor] 201-201: Too few public methods (0/2)
(R0903)
🪛 Ruff (0.11.9)
backend/app/groups/service.py
50-50: Local variable e is assigned to but never used
Remove assignment to unused variable e
(F841)
🔇 Additional comments (2)
backend/app/groups/service.py (2)
145-157: Excellent use of the new enrichment method!The refactoring successfully eliminates the duplicated member enrichment logic in
get_group_by_id. The implementation is clean and maintains the same functionality while improving maintainability.
280-285: Perfect application of the DRY principle!The
get_group_membersmethod now uses the shared enrichment logic, completing the refactoring to eliminate code duplication. This makes the codebase more maintainable and consistent.
| tags: Optional[List[str]] = [] | ||
| receiptUrls: Optional[List[str]] = [] |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Fix additional mutable default values not previously flagged.
Several models still use mutable list defaults that weren't covered in previous reviews.
- tags: Optional[List[str]] = []
- receiptUrls: Optional[List[str]] = []
+ tags: Optional[List[str]] = Field(default_factory=list)
+ receiptUrls: Optional[List[str]] = Field(default_factory=list)- tags: List[str] = []
- receiptUrls: List[str] = []
+ tags: List[str] = Field(default_factory=list)
+ receiptUrls: List[str] = Field(default_factory=list)- consolidatedExpenses: Optional[List[str]] = []
+ consolidatedExpenses: Optional[List[str]] = Field(default_factory=list)- pendingSettlements: List[Settlement] = []
- recentExpenses: List[Dict[str, Any]] = []
+ pendingSettlements: List[Settlement] = Field(default_factory=list)
+ recentExpenses: List[Dict[str, Any]] = Field(default_factory=list)Also applies to: 87-88, 118-118, 159-160
🤖 Prompt for AI Agents
In backend/app/expenses/schemas.py at lines 26-27, 87-88, 118, and 159-160,
mutable default values like empty lists are used directly in model fields, which
can lead to shared state bugs. Replace these mutable defaults with None and use
a default_factory or initialize the lists properly to avoid shared mutable
defaults in Pydantic models.
| model_config = { | ||
| # "populate_by_name": True, | ||
| "str_strip_whitespace": True, | ||
| "validate_assignment": True | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Standardize model configuration across all schema classes.
Model configuration is inconsistent across classes. Some models have comprehensive configs while others have minimal or no configuration.
Consider standardizing the configuration for all models that need it:
# For models that need MongoDB field mapping
class ExpenseComment(BaseModel):
# ... fields ...
model_config = {
- # "populate_by_name": True,
+ "populate_by_name": True,
"str_strip_whitespace": True,
"validate_assignment": True
}# For other models, add consistent configuration
class OptimizedSettlement(BaseModel):
# ... fields ...
+
+ model_config = {
+ "str_strip_whitespace": True,
+ "validate_assignment": True
+ }Apply similar configurations to models that currently lack model_config: OptimizedSettlement, GroupSummary, ExpenseCreateResponse, ExpenseListResponse, SettlementCreateRequest, SettlementUpdateRequest, SettlementListResponse, UserBalance, FriendBalanceBreakdown, FriendBalance, FriendsBalanceResponse, BalanceSummaryResponse, ExpenseAnalytics, AttachmentUploadResponse, and OptimizedSettlementsResponse.
Also applies to: 77-77, 94-94, 110-110
🤖 Prompt for AI Agents
In backend/app/expenses/schemas.py around lines 64-68, 77, 94, and 110, the
model_config settings are inconsistent across schema classes. To fix this, add a
standardized model_config dictionary with keys like "str_strip_whitespace": True
and "validate_assignment": True to all the listed schema classes that currently
lack it, ensuring uniform configuration across OptimizedSettlement,
GroupSummary, ExpenseCreateResponse, ExpenseListResponse,
SettlementCreateRequest, SettlementUpdateRequest, SettlementListResponse,
UserBalance, FriendBalanceBreakdown, FriendBalance, FriendsBalanceResponse,
BalanceSummaryResponse, ExpenseAnalytics, AttachmentUploadResponse, and
OptimizedSettlementsResponse.
| class SettlementCreateRequest(BaseModel): | ||
| payer_id: str | ||
| payee_id: str | ||
| amount: float = Field(..., gt=0) | ||
| description: Optional[str] = None | ||
| paidAt: Optional[datetime] = None |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Inconsistent field naming convention.
The SettlementCreateRequest uses snake_case field names (payer_id, payee_id) while other models consistently use camelCase.
class SettlementCreateRequest(BaseModel):
- payer_id: str
- payee_id: str
+ payerId: str
+ payeeId: str
amount: float = Field(..., gt=0)
description: Optional[str] = None
paidAt: Optional[datetime] = None📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| class SettlementCreateRequest(BaseModel): | |
| payer_id: str | |
| payee_id: str | |
| amount: float = Field(..., gt=0) | |
| description: Optional[str] = None | |
| paidAt: Optional[datetime] = None | |
| class SettlementCreateRequest(BaseModel): | |
| payerId: str | |
| payeeId: str | |
| amount: float = Field(..., gt=0) | |
| description: Optional[str] = None | |
| paidAt: Optional[datetime] = None |
🧰 Tools
🪛 Flake8 (7.2.0)
[error] 135-135: expected 2 blank lines, found 1
(E302)
🪛 Pylint (3.3.7)
[refactor] 135-135: Too few public methods (0/2)
(R0903)
🤖 Prompt for AI Agents
In backend/app/expenses/schemas.py around lines 135 to 140, the field names in
SettlementCreateRequest use snake_case (payer_id, payee_id) which is
inconsistent with the camelCase naming convention used in other models. Rename
these fields to camelCase (payerId, payeeId) to maintain consistency across the
codebase.
| createdAt: datetime | ||
|
|
||
| model_config = { | ||
| # "populate_by_name": True, |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enable populate_by_name for MongoDB compatibility.
The populate_by_name configuration is commented out but appears necessary for proper MongoDB field alias handling.
model_config = {
- # "populate_by_name": True,
+ "populate_by_name": True,
"str_strip_whitespace": True,
"validate_assignment": True
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| # "populate_by_name": True, | |
| model_config = { | |
| "populate_by_name": True, | |
| "str_strip_whitespace": True, | |
| "validate_assignment": True | |
| } |
🤖 Prompt for AI Agents
In backend/app/expenses/schemas.py at line 65, the populate_by_name setting is
commented out but needed for MongoDB compatibility to handle field aliases
correctly. Uncomment the "populate_by_name": True line in the relevant model or
schema configuration to enable this feature.
|
Made changes. Now let's run the tests one final time: Excellent! All tests are now passing! Let's run the full test suite to make sure we didn't break anything else: Perfect! All 72 tests are now passing! 🎉 SummaryI successfully analyzed and fixed all the errors in the expense service tests by identifying and addressing the root causes: Root Cause Analysis
Key Changes MadeService Layer (service.py):# Added ObjectId validation in create_expense()
try:
group_obj_id = ObjectId(group_id)
except Exception:
raise ValueError("Group not found or user not a member")
# Added ObjectId validation in get_expense_by_id()
try:
group_obj_id = ObjectId(group_id)
expense_obj_id = ObjectId(expense_id)
except Exception:
raise ValueError("Group not found or user not a member")Test Layer (test_expense_service.py):
Testing Patterns LearnedBy analyzing the groups and user service tests, I learned the correct patterns:
The solution now follows the same testing patterns as the other services and properly handles edge cases like invalid ObjectIds, which makes the codebase more robust and consistent. |
…al in create_expense and get_expense_by_id methods
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (5)
backend/tests/expenses/test_expense_service.py (1)
7-7: Remove unused import.The
asyncioimport is not used in this test file.-import asynciobackend/app/expenses/service.py (4)
1-10: Remove unused imports.Several imports are not used in the code and should be removed.
-from typing import List, Dict, Any, Optional, Tuple +from typing import List, Dict, Any, Optional from datetime import datetime, timedelta from bson import ObjectId from app.database import mongodb from app.expenses.schemas import ( - ExpenseCreateRequest, ExpenseUpdateRequest, ExpenseResponse, Settlement, - OptimizedSettlement, SettlementCreateRequest, SettlementStatus, SplitType + ExpenseCreateRequest, ExpenseUpdateRequest, ExpenseResponse, Settlement, + OptimizedSettlement, SettlementCreateRequest, SettlementStatus ) -import asyncio -from collections import defaultdict, deque +from collections import defaultdict
245-248: Use the exception variable in the error message.The exception is caught but not used in the error message.
try: expense_obj_id = ObjectId(expense_id) except Exception as e: - raise ValueError(f"Invalid expense ID format: {expense_id}") + raise ValueError(f"Invalid expense ID format: {expense_id}") from e
294-300: Avoid bare except clause.Bare
except:catches all exceptions including system exceptions likeKeyboardInterrupt.try: user = await self.users_collection.find_one({"_id": ObjectId(user_id)}) user_name = user.get("name", "Unknown User") if user else "Unknown User" -except: +except Exception: user_name = "Unknown User"
344-344: Use proper logging instead of print statements.Replace print statements with Python's logging module for better production-ready logging.
#!/bin/bash # Check if logging is used elsewhere in the codebase to understand the pattern echo "Checking for existing logging usage patterns..." rg -A 2 "import logging|logger\s*=" --type py | head -20 echo -e "\nChecking for other print statements that might need replacement..." rg "print\(" --type py -C 2 | grep -E "error|warning|debug|info" | head -10Also applies to: 357-360
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
backend/app/expenses/service.py(1 hunks)backend/tests/expenses/test_expense_service.py(1 hunks)
🧰 Additional context used
🪛 Ruff (0.11.9)
backend/tests/expenses/test_expense_service.py
7-7: asyncio imported but unused
Remove unused import: asyncio
(F401)
backend/app/expenses/service.py
1-1: typing.Tuple imported but unused
Remove unused import: typing.Tuple
(F401)
7-7: app.expenses.schemas.SplitType imported but unused
Remove unused import: app.expenses.schemas.SplitType
(F401)
9-9: asyncio imported but unused
Remove unused import: asyncio
(F401)
10-10: collections.deque imported but unused
Remove unused import: collections.deque
(F401)
39-39: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
203-203: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
247-247: Local variable e is assigned to but never used
Remove assignment to unused variable e
(F841)
248-248: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
299-299: Do not use bare except
(E722)
360-360: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
🪛 Flake8 (7.2.0)
backend/tests/expenses/test_expense_service.py
[error] 7-7: 'asyncio' imported but unused
(F401)
[error] 9-9: expected 2 blank lines, found 1
(E302)
[error] 15-15: expected 2 blank lines, found 1
(E302)
[error] 28-28: expected 2 blank lines, found 1
(E302)
[error] 50-50: expected 2 blank lines, found 1
(E302)
[error] 92-92: expected 2 blank lines, found 1
(E302)
[error] 114-114: expected 2 blank lines, found 1
(E302)
[error] 185-185: expected 2 blank lines, found 1
(E302)
[error] 253-253: expected 2 blank lines, found 1
(E302)
[error] 295-295: expected 2 blank lines, found 1
(E302)
[error] 317-317: expected 2 blank lines, found 1
(E302)
[error] 349-349: expected 2 blank lines, found 1
(E302)
[error] 388-388: expected 2 blank lines, found 1
(E302)
[error] 415-415: expected 2 blank lines, found 1
(E302)
backend/app/expenses/service.py
[error] 1-1: 'typing.Tuple' imported but unused
(F401)
[error] 5-5: 'app.expenses.schemas.SplitType' imported but unused
(F401)
[error] 9-9: 'asyncio' imported but unused
(F401)
[error] 10-10: 'collections.deque' imported but unused
(F401)
[error] 12-12: expected 2 blank lines, found 1
(E302)
[error] 125-125: continuation line under-indented for visual indent
(E128)
[error] 126-126: continuation line under-indented for visual indent
(E128)
[error] 247-247: local variable 'e' is assigned to but never used
(F841)
[error] 299-299: do not use bare 'except'
(E722)
[error] 586-586: continuation line under-indented for visual indent
(E128)
[error] 649-649: continuation line under-indented for visual indent
(E128)
[error] 992-992: continuation line under-indented for visual indent
(E128)
[error] 1099-1099: expected 2 blank lines after class or function definition, found 0
(E305)
🪛 Pylint (3.3.7)
backend/app/expenses/service.py
[refactor] 124-124: Too many arguments (8/5)
(R0913)
[refactor] 124-124: Too many positional arguments (8/5)
(R0917)
[refactor] 124-124: Too many local variables (21/15)
(R0914)
[refactor] 240-240: Too many local variables (18/15)
(R0914)
[refactor] 240-240: Too many branches (22/12)
(R0912)
[refactor] 240-240: Too many statements (63/50)
(R0915)
[refactor] 384-387: Unnecessary "else" after "return", remove the "else" and de-indent the code inside it
(R1705)
[refactor] 441-441: Too many local variables (21/15)
(R0914)
[refactor] 585-585: Too many arguments (6/5)
(R0913)
[refactor] 585-585: Too many positional arguments (6/5)
(R0917)
[refactor] 648-648: Too many arguments (6/5)
(R0913)
[refactor] 648-648: Too many positional arguments (6/5)
(R0917)
[refactor] 694-694: Too many local variables (22/15)
(R0914)
[refactor] 799-799: Too many local variables (19/15)
(R0914)
[refactor] 991-991: Too many arguments (6/5)
(R0913)
[refactor] 991-991: Too many positional arguments (6/5)
(R0917)
[refactor] 991-991: Too many local variables (31/15)
(R0914)
[refactor] 991-991: Too many branches (16/12)
(R0912)
🔇 Additional comments (10)
backend/tests/expenses/test_expense_service.py (6)
50-91: Well-structured test with comprehensive mocking.The test properly mocks all database operations and service methods, and validates the response structure correctly.
92-113: Good edge case coverage for invalid group scenarios.The test properly handles both invalid ObjectId format and non-existent group cases with appropriate error validation.
114-184: Excellent test for advanced settlement optimization.The test creates a realistic scenario where transitive debts are optimized (B→A and C→B becomes C→A), and correctly validates the optimization results.
185-252: Good test for normal settlement algorithm.The test properly validates the netting of reciprocal debts and handles the expected behavior of the normal algorithm.
317-348: Comprehensive validation testing for expense splits.The test properly validates both successful creation and error cases when splits don't sum to the total amount.
349-387: Good coverage of different split types with floating-point tolerance.The test correctly handles both equal and unequal splits, and properly accounts for floating-point precision when validating totals.
backend/app/expenses/service.py (4)
389-439: Well-implemented normal settlement algorithm.The algorithm correctly simplifies direct relationships between users by netting out reciprocal debts. The floating-point comparison with 0.01 tolerance is appropriate for financial calculations.
441-513: Excellent implementation of advanced settlement optimization.The algorithm uses a graph-based approach with a two-pointer technique to minimize the total number of transactions. It correctly:
- Calculates net balances for each user
- Separates debtors and creditors
- Matches them optimally to minimize transactions
- Handles floating-point precision appropriately
This is a solid implementation of the debt simplification problem.
799-917: Comprehensive implementation of cross-group friend balances.The method effectively:
- Identifies all friends across groups
- Calculates net balances per group
- Aggregates totals across all groups
- Provides detailed breakdowns for transparency
This is a valuable feature for users who share expenses across multiple groups.
991-1097: Comprehensive analytics implementation.The method provides valuable insights including:
- Expense trends over time
- Category analysis with percentages
- Member contribution tracking
- Proper date range handling for monthly/yearly periods
This gives users excellent visibility into their group's spending patterns.
| try: | ||
| group_obj_id = ObjectId(group_id) | ||
| except Exception: | ||
| raise ValueError("Group not found or user not a member") | ||
|
|
There was a problem hiding this comment.
Improve exception handling for ObjectId validation.
The exception should be chained and provide a more specific error message.
# Validate and convert group_id to ObjectId
try:
group_obj_id = ObjectId(group_id)
-except Exception:
- raise ValueError("Group not found or user not a member")
+except Exception as e:
+ raise ValueError(f"Invalid group ID format: {group_id}") from e📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| try: | |
| group_obj_id = ObjectId(group_id) | |
| except Exception: | |
| raise ValueError("Group not found or user not a member") | |
| # Validate and convert group_id to ObjectId | |
| try: | |
| group_obj_id = ObjectId(group_id) | |
| except Exception as e: | |
| raise ValueError(f"Invalid group ID format: {group_id}") from e |
🧰 Tools
🪛 Ruff (0.11.9)
39-39: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
🤖 Prompt for AI Agents
In backend/app/expenses/service.py around lines 36 to 40, the current exception
handling for ObjectId conversion catches a generic Exception and raises a
ValueError without chaining the original exception or providing a specific error
message. Modify the except block to catch the specific exception thrown by
ObjectId conversion, chain the original exception using "from" in the raise
statement, and update the error message to be more descriptive about the failure
reason.
| try: | ||
| group_obj_id = ObjectId(group_id) | ||
| expense_obj_id = ObjectId(expense_id) | ||
| except Exception: | ||
| raise ValueError("Group not found or user not a member") | ||
|
|
There was a problem hiding this comment.
Improve exception handling for ObjectId validation.
The exception should be chained and provide more specific error messages.
# Validate ObjectIds
try:
group_obj_id = ObjectId(group_id)
expense_obj_id = ObjectId(expense_id)
-except Exception:
- raise ValueError("Group not found or user not a member")
+except Exception as e:
+ raise ValueError(f"Invalid ID format: group_id={group_id}, expense_id={expense_id}") from e📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| try: | |
| group_obj_id = ObjectId(group_id) | |
| expense_obj_id = ObjectId(expense_id) | |
| except Exception: | |
| raise ValueError("Group not found or user not a member") | |
| # Validate ObjectIds | |
| try: | |
| group_obj_id = ObjectId(group_id) | |
| expense_obj_id = ObjectId(expense_id) | |
| except Exception as e: | |
| raise ValueError(f"Invalid ID format: group_id={group_id}, expense_id={expense_id}") from e |
🧰 Tools
🪛 Ruff (0.11.9)
203-203: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
🤖 Prompt for AI Agents
In backend/app/expenses/service.py around lines 199 to 204, the current
exception handling for ObjectId conversion catches a generic Exception and
raises a ValueError without chaining the original exception or providing
specific error details. Update the except block to catch the specific exception
type raised by ObjectId conversion, chain the original exception using "from" in
the raise statement, and provide a more precise error message indicating whether
the group_id or expense_id is invalid.
Adds comprehensive tests for various methods in the ExpenseService class, including: - list_group_expenses (with filters and pagination) - delete_expense - create_manual_settlement - get_group_settlements (with filters and pagination) - get_settlement_by_id - update_settlement_status - delete_settlement - get_user_balance_in_group - get_friends_balance_summary - get_overall_balance_summary - get_group_analytics These tests cover success cases, error handling, and edge cases to improve the robustness and reliability of the expense service. Fixes several issues in existing tests related to ObjectId handling and mocking of async database operations. Co-authored-by: google-labs-jules[bot] <161369871+google-labs-jules[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
backend/tests/expenses/test_expense_service.py (1)
7-7: Remove unused import.The
asynciomodule is imported but never used in the test file.-import asyncio
🧹 Nitpick comments (3)
backend/tests/expenses/test_expense_service.py (3)
10-13: Fix misleading docstring in the fixture.The docstring mentions "with mocked database" but the fixture doesn't actually mock the database. The mocking is done in individual tests.
def expense_service(): - """Create an ExpenseService instance with mocked database""" + """Create an ExpenseService instance for testing""" service = ExpenseService() return service
1297-1298: Split multiple statements into separate lines.Multiple statements on one line violate PEP 8 style guidelines.
- if friend1_id_obj in ids_in_query: users_to_return.append(mock_friend1_doc) - if friend2_id_obj in ids_in_query: users_to_return.append(mock_friend2_doc) + if friend1_id_obj in ids_in_query: + users_to_return.append(mock_friend1_doc) + if friend2_id_obj in ids_in_query: + users_to_return.append(mock_friend2_doc)
1543-1545: Split multiple statements into separate lines.Multiple statements on one line violate PEP 8 style guidelines.
- if user_id_query_obj == user_a_obj: return mock_user_a_doc_db - if user_id_query_obj == user_b_obj: return mock_user_b_doc_db - if user_id_query_obj == user_c_obj: return mock_user_c_doc_db + if user_id_query_obj == user_a_obj: + return mock_user_a_doc_db + if user_id_query_obj == user_b_obj: + return mock_user_b_doc_db + if user_id_query_obj == user_c_obj: + return mock_user_c_doc_db
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
backend/tests/expenses/test_expense_service.py(1 hunks)
🧰 Additional context used
🪛 Ruff (0.11.9)
backend/tests/expenses/test_expense_service.py
7-7: asyncio imported but unused
Remove unused import: asyncio
(F401)
1227-1227: Local variable mock_user_main_doc is assigned to but never used
Remove assignment to unused variable mock_user_main_doc
(F841)
1297-1297: Multiple statements on one line (colon)
(E701)
1298-1298: Multiple statements on one line (colon)
(E701)
1543-1543: Multiple statements on one line (colon)
(E701)
1544-1544: Multiple statements on one line (colon)
(E701)
1545-1545: Multiple statements on one line (colon)
(E701)
🪛 Flake8 (7.2.0)
backend/tests/expenses/test_expense_service.py
[error] 7-7: 'asyncio' imported but unused
(F401)
[error] 9-9: expected 2 blank lines, found 1
(E302)
[error] 15-15: expected 2 blank lines, found 1
(E302)
[error] 28-28: expected 2 blank lines, found 1
(E302)
[error] 50-50: expected 2 blank lines, found 1
(E302)
[error] 92-92: expected 2 blank lines, found 1
(E302)
[error] 114-114: expected 2 blank lines, found 1
(E302)
[error] 185-185: expected 2 blank lines, found 1
(E302)
[error] 253-253: expected 2 blank lines, found 1
(E302)
[error] 295-295: expected 2 blank lines, found 1
(E302)
[error] 317-317: expected 2 blank lines, found 1
(E302)
[error] 349-349: expected 2 blank lines, found 1
(E302)
[error] 388-388: expected 2 blank lines, found 1
(E302)
[error] 415-415: expected 2 blank lines, found 1
(E302)
[error] 431-431: expected 2 blank lines, found 1
(E302)
[error] 469-469: expected 2 blank lines, found 1
(E302)
[error] 479-479: at least two spaces before inline comment
(E261)
[error] 484-484: at least two spaces before inline comment
(E261)
[error] 494-494: expected 2 blank lines, found 1
(E302)
[error] 504-504: at least two spaces before inline comment
(E261)
[error] 509-509: at least two spaces before inline comment
(E261)
[error] 525-525: at least two spaces before inline comment
(E261)
[error] 589-589: at least two spaces before inline comment
(E261)
[error] 594-594: expected 2 blank lines, found 1
(E302)
[error] 615-615: at least two spaces before inline comment
(E261)
[error] 629-629: expected 2 blank lines, found 1
(E302)
[error] 632-632: at least two spaces before inline comment
(E261)
[error] 633-633: at least two spaces before inline comment
(E261)
[error] 634-634: at least two spaces before inline comment
(E261)
[error] 643-643: at least two spaces before inline comment
(E261)
[error] 644-644: at least two spaces before inline comment
(E261)
[error] 652-652: expected 2 blank lines, found 1
(E302)
[error] 666-666: at least two spaces before inline comment
(E261)
[error] 673-673: at least two spaces before inline comment
(E261)
[error] 674-674: at least two spaces before inline comment
(E261)
[error] 677-677: expected 2 blank lines, found 1
(E302)
[error] 683-683: at least two spaces before inline comment
(E261)
[error] 717-717: at least two spaces before inline comment
(E261)
[error] 718-718: at least two spaces before inline comment
(E261)
[error] 719-719: at least two spaces before inline comment
(E261)
[error] 736-736: at least two spaces before inline comment
(E261)
[error] 747-747: at least two spaces before inline comment
(E261)
[error] 749-749: expected 2 blank lines, found 1
(E302)
[error] 754-754: at least two spaces before inline comment
(E261)
[error] 757-757: at least two spaces before inline comment
(E261)
[error] 758-758: at least two spaces before inline comment
(E261)
[error] 765-765: at least two spaces before inline comment
(E261)
[error] 772-772: expected 2 blank lines, found 1
(E302)
[error] 811-811: at least two spaces before inline comment
(E261)
[error] 837-837: at least two spaces before inline comment
(E261)
[error] 839-839: at least two spaces before inline comment
(E261)
[error] 862-862: expected 2 blank lines, found 1
(E302)
[error] 865-865: at least two spaces before inline comment
(E261)
[error] 871-871: at least two spaces before inline comment
(E261)
[error] 879-879: expected 2 blank lines, found 1
(E302)
[error] 904-904: at least two spaces before inline comment
(E261)
[error] 917-917: expected 2 blank lines, found 1
(E302)
[error] 922-922: at least two spaces before inline comment
(E261)
[error] 929-929: at least two spaces before inline comment
(E261)
[error] 934-934: expected 2 blank lines, found 1
(E302)
[error] 945-945: at least two spaces before inline comment
(E261)
[error] 952-952: expected 2 blank lines, found 1
(E302)
[error] 973-973: at least two spaces before inline comment
(E261)
[error] 991-991: at least two spaces before inline comment
(E261)
[error] 997-997: at least two spaces before inline comment
(E261)
[error] 1006-1006: expected 2 blank lines, found 1
(E302)
[error] 1012-1012: at least two spaces before inline comment
(E261)
[error] 1020-1020: at least two spaces before inline comment
(E261)
[error] 1026-1026: too many blank lines (2)
(E303)
[error] 1031-1031: at least two spaces before inline comment
(E261)
[error] 1033-1033: expected 2 blank lines, found 1
(E302)
[error] 1037-1037: at least two spaces before inline comment
(E261)
[error] 1065-1065: expected 2 blank lines, found 1
(E302)
[error] 1070-1070: at least two spaces before inline comment
(E261)
[error] 1079-1079: at least two spaces before inline comment
(E261)
[error] 1086-1086: expected 2 blank lines, found 1
(E302)
[error] 1097-1097: at least two spaces before inline comment
(E261)
[error] 1104-1104: expected 2 blank lines, found 1
(E302)
[error] 1110-1110: at least two spaces before inline comment
(E261)
[error] 1121-1121: at least two spaces before inline comment
(E261)
[error] 1128-1128: at least two spaces before inline comment
(E261)
[error] 1154-1154: at least two spaces before inline comment
(E261)
[error] 1163-1163: too many blank lines (2)
(E303)
[error] 1170-1170: at least two spaces before inline comment
(E261)
[error] 1171-1171: at least two spaces before inline comment
(E261)
[error] 1188-1188: at least two spaces before inline comment
(E261)
[error] 1191-1191: at least two spaces before inline comment
(E261)
[error] 1198-1198: at least two spaces before inline comment
(E261)
[error] 1199-1199: at least two spaces before inline comment
(E261)
[error] 1204-1204: at least two spaces before inline comment
(E261)
[error] 1214-1214: expected 2 blank lines, found 1
(E302)
[error] 1224-1224: at least two spaces before inline comment
(E261)
[error] 1227-1227: local variable 'mock_user_main_doc' is assigned to but never used
(F841)
[error] 1280-1280: at least two spaces before inline comment
(E261)
[error] 1295-1295: at least two spaces before inline comment
(E261)
[error] 1297-1297: multiple statements on one line (colon)
(E701)
[error] 1298-1298: multiple statements on one line (colon)
(E701)
[error] 1310-1310: too many blank lines (2)
(E303)
[error] 1319-1319: at least two spaces before inline comment
(E261)
[error] 1332-1332: at least two spaces before inline comment
(E261)
[error] 1340-1340: at least two spaces before inline comment
(E261)
[error] 1347-1347: too many blank lines (2)
(E303)
[error] 1381-1381: at least two spaces before inline comment
(E261)
[error] 1383-1383: at least two spaces before inline comment
(E261)
[error] 1398-1398: expected 2 blank lines, found 1
(E302)
[error] 1404-1404: at least two spaces before inline comment
(E261)
[error] 1428-1428: at least two spaces before inline comment
(E261)
[error] 1430-1430: at least two spaces before inline comment
(E261)
[error] 1474-1474: at least two spaces before inline comment
(E261)
[error] 1476-1476: expected 2 blank lines, found 1
(E302)
[error] 1486-1486: at least two spaces before inline comment
(E261)
[error] 1489-1489: at least two spaces before inline comment
(E261)
[error] 1499-1499: expected 2 blank lines, found 1
(E302)
[error] 1502-1502: at least two spaces before inline comment
(E261)
[error] 1503-1503: at least two spaces before inline comment
(E261)
[error] 1505-1505: at least two spaces before inline comment
(E261)
[error] 1542-1542: at least two spaces before inline comment
(E261)
[error] 1543-1543: multiple statements on one line (colon)
(E701)
[error] 1544-1544: multiple statements on one line (colon)
(E701)
[error] 1545-1545: multiple statements on one line (colon)
(E701)
[error] 1555-1555: at least two spaces before inline comment
(E261)
[error] 1565-1565: too many blank lines (2)
(E303)
[error] 1570-1570: at least two spaces before inline comment
(E261)
[error] 1582-1582: at least two spaces before inline comment
(E261)
[error] 1601-1601: at least two spaces before inline comment
(E261)
[error] 1627-1627: at least two spaces before inline comment
(E261)
[error] 1632-1632: at least two spaces before inline comment
(E261)
[error] 1645-1645: at least two spaces before inline comment
(E261)
[error] 1651-1651: at least two spaces before inline comment
(E261)
🪛 Pylint (3.3.7)
backend/tests/expenses/test_expense_service.py
[refactor] 678-678: Too many local variables (17/15)
(R0914)
[refactor] 1105-1105: Too many local variables (18/15)
(R0914)
[refactor] 1215-1215: Too many local variables (23/15)
(R0914)
[refactor] 1262-1267: Unnecessary "elif" after "break", remove the leading "el" from "elif"
(R1723)
[refactor] 1215-1215: Too many statements (74/50)
(R0915)
[refactor] 1500-1500: Too many local variables (34/15)
(R0914)
[refactor] 1500-1500: Too many statements (72/50)
(R0915)
🔇 Additional comments (1)
backend/tests/expenses/test_expense_service.py (1)
1-1661: Well-structured and comprehensive test coverage!The test file provides excellent coverage for the Expense Service with proper async testing patterns, comprehensive mocking, and good coverage of success, error, and edge cases. While some tests exceed complexity thresholds (particularly
test_get_friends_balance_summary_successandtest_get_group_analytics_success), the complexity appears justified given the intricate business logic being tested.The consistent mocking patterns and descriptive test names make the test suite maintainable and easy to understand.
| group1_id = str(ObjectId()) # Remains as string, used for direct comparison in mock | ||
| group2_id = str(ObjectId()) | ||
|
|
||
| mock_user_main_doc = {"_id": user_id_obj, "name": "Main User"} |
There was a problem hiding this comment.
Remove unused variable.
The mock_user_main_doc variable is defined but never used in the test.
- mock_user_main_doc = {"_id": user_id_obj, "name": "Main User"}
mock_friend1_doc = {"_id": friend1_id_obj, "name": "Friend One"}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| mock_user_main_doc = {"_id": user_id_obj, "name": "Main User"} | |
| mock_friend1_doc = {"_id": friend1_id_obj, "name": "Friend One"} |
🧰 Tools
🪛 Ruff (0.11.9)
1227-1227: Local variable mock_user_main_doc is assigned to but never used
Remove assignment to unused variable mock_user_main_doc
(F841)
🪛 Flake8 (7.2.0)
[error] 1227-1227: local variable 'mock_user_main_doc' is assigned to but never used
(F841)
🤖 Prompt for AI Agents
In backend/tests/expenses/test_expense_service.py at line 1227, the variable
mock_user_main_doc is defined but not used anywhere in the test. Remove the
entire line defining mock_user_main_doc to clean up unused code.
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Tests
Refactor